implemented annotation merging on SimpleDeviceStore.

- Added annotation support to PortsDescriptions Change-Id: I157e4fb93b8f387b405722b8d004501d993decda

implemented annotation merging on SimpleDeviceStore.
- Added annotation support to PortsDescriptions Change-Id: I157e4fb93b8f387b405722b8d004501d993decda
Yuta HIGUCHI
Commit 55710e70c0c8276b79d80e8b389afb810ae36521 55710e70 1 parent b876bf1f
Showing 6 changed files with 237 additions and 34 deletions
core/api/src/main/java/org/onlab/onos/net/DefaultAnnotations.java
core/api/src/main/java/org/onlab/onos/net/device/DefaultDeviceDescription.java
core/api/src/main/java/org/onlab/onos/net/device/DefaultPortDescription.java
core/api/src/main/java/org/onlab/onos/net/device/PortDescription.java
core/store/trivial/src/main/java/org/onlab/onos/store/trivial/impl/SimpleDeviceStore.java
core/store/trivial/src/test/java/org/onlab/onos/store/trivial/impl/SimpleDeviceStoreTest.java
--- a/core/api/src/main/java/org/onlab/onos/net/DefaultAnnotations.java
View file @55710e7
+++ b/core/api/src/main/java/org/onlab/onos/net/DefaultAnnotations.java
View file @55710e7
 package org.onlab.onos.net;
+import java.util.Collections;
 import java.util.HashMap;
 import java.util.Map;
 import java.util.Objects;
@@ -71,9 +72,33 @@ public final class DefaultAnnotations implements SparseAnnotations {
         return new DefaultAnnotations(merged);
     }
+    /**
+     * Convert Annotations to DefaultAnnotations if needed and merges.
+     *
+     * @see #merge(DefaultAnnotations, SparseAnnotations)
+     *
+     * @param annotations       base annotations
+     * @param sparseAnnotations additional sparse annotations
+     * @return combined annotations or the original base annotations if there
+     * are not additional annotations
+     */
+    public static DefaultAnnotations merge(Annotations annotations,
+                                    SparseAnnotations sparseAnnotations) {
+        if (annotations instanceof DefaultAnnotations) {
+            return merge((DefaultAnnotations) annotations, sparseAnnotations);
+        }
+
+        DefaultAnnotations.Builder builder = DefaultAnnotations.builder();
+        for (String key : annotations.keys()) {
+            builder.set(key, annotations.value(key));
+        }
+        return merge(builder.build(), sparseAnnotations);
+    }
+
     @Override
     public Set<String> keys() {
-        return map.keySet();
+        // TODO: unmodifiable to be removed after switching to ImmutableMap;
+        return Collections.unmodifiableSet(map.keySet());
     }
     @Override
--- a/core/api/src/main/java/org/onlab/onos/net/device/DefaultDeviceDescription.java
View file @55710e7
+++ b/core/api/src/main/java/org/onlab/onos/net/device/DefaultDeviceDescription.java
View file @55710e7
@@ -45,6 +45,18 @@ public class DefaultDeviceDescription extends AbstractDescription
         this.serialNumber = serialNumber;
     }
+    /**
+     * Creates a device description using the supplied information.
+     * @param base DeviceDescription to basic information
+     * @param annotations Annotations to use.
+     */
+    public DefaultDeviceDescription(DeviceDescription base,
+                                    SparseAnnotations... annotations) {
+        this(base.deviceURI(), base.type(), base.manufacturer(),
+             base.hwVersion(), base.swVersion(), base.serialNumber(),
+             annotations);
+    }
+
     @Override
     public URI deviceURI() {
         return uri;
--- a/core/api/src/main/java/org/onlab/onos/net/device/DefaultPortDescription.java
View file @55710e7
+++ b/core/api/src/main/java/org/onlab/onos/net/device/DefaultPortDescription.java
View file @55710e7
 package org.onlab.onos.net.device;
+import org.onlab.onos.net.AbstractDescription;
 import org.onlab.onos.net.PortNumber;
+import org.onlab.onos.net.SparseAnnotations;
 /**
  * Default implementation of immutable port description.
  */
-public class DefaultPortDescription implements PortDescription {
+public class DefaultPortDescription extends AbstractDescription
+        implements PortDescription {
     private final PortNumber number;
     private final boolean isEnabled;
-    public DefaultPortDescription(PortNumber number, boolean isEnabled) {
+    /**
+     * Creates a port description using the supplied information.
+     *
+     * @param number       port number
+     * @param isEnabled    port enabled state
+     * @param annotations  optional key/value annotations map
+     */
+    public DefaultPortDescription(PortNumber number, boolean isEnabled,
+                SparseAnnotations... annotations) {
+        super(annotations);
         this.number = number;
         this.isEnabled = isEnabled;
     }
+    /**
+     * Creates a port description using the supplied information.
+     *
+     * @param base         PortDescription to get basic information from
+     * @param annotations  optional key/value annotations map
+     */
+    public DefaultPortDescription(PortDescription base,
+            SparseAnnotations annotations) {
+        this(base.portNumber(), base.isEnabled(), annotations);
+    }
+
     @Override
     public PortNumber portNumber() {
         return number;
--- a/core/api/src/main/java/org/onlab/onos/net/device/PortDescription.java
View file @55710e7
+++ b/core/api/src/main/java/org/onlab/onos/net/device/PortDescription.java
View file @55710e7
 package org.onlab.onos.net.device;
+import org.onlab.onos.net.Description;
 import org.onlab.onos.net.PortNumber;
 /**
  * Information about a port.
  */
-public interface PortDescription {
+public interface PortDescription extends Description {
     // TODO: possibly relocate this to a common ground so that this can also used by host tracking if required
--- a/core/store/trivial/src/main/java/org/onlab/onos/store/trivial/impl/SimpleDeviceStore.java
View file @55710e7
+++ b/core/store/trivial/src/main/java/org/onlab/onos/store/trivial/impl/SimpleDeviceStore.java
View file @55710e7
@@ -9,6 +9,8 @@ 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.DefaultAnnotations;
 import org.onlab.onos.net.DefaultDevice;
 import org.onlab.onos.net.DefaultPort;
 import org.onlab.onos.net.Device;
@@ -16,6 +18,9 @@ import org.onlab.onos.net.Device.Type;
 import org.onlab.onos.net.DeviceId;
 import org.onlab.onos.net.Port;
 import org.onlab.onos.net.PortNumber;
+import org.onlab.onos.net.SparseAnnotations;
+import org.onlab.onos.net.device.DefaultDeviceDescription;
+import org.onlab.onos.net.device.DefaultPortDescription;
 import org.onlab.onos.net.device.DeviceDescription;
 import org.onlab.onos.net.device.DeviceEvent;
 import org.onlab.onos.net.device.DeviceStore;
@@ -45,6 +50,7 @@ import static com.google.common.base.Predicates.notNull;
 import static org.onlab.onos.net.device.DeviceEvent.Type.*;
 import static org.slf4j.LoggerFactory.getLogger;
 import static org.apache.commons.lang3.concurrent.ConcurrentUtils.createIfAbsentUnchecked;
+import static org.onlab.onos.net.DefaultAnnotations.merge;
 // TODO: synchronization should be done in more fine-grained manner.
 /**
@@ -112,8 +118,8 @@ public class SimpleDeviceStore
             = createIfAbsentUnchecked(providerDescs, providerId,
                     new InitDeviceDescs(deviceDescription));
+        // update description
         descs.putDeviceDesc(deviceDescription);
-
         Device newDevice = composeDevice(deviceId, providerDescs);
         if (oldDevice == null) {
@@ -144,7 +150,8 @@ public class SimpleDeviceStore
         // We allow only certain attributes to trigger update
         if (!Objects.equals(oldDevice.hwVersion(), newDevice.hwVersion()) ||
-            !Objects.equals(oldDevice.swVersion(), newDevice.swVersion())) {
+            !Objects.equals(oldDevice.swVersion(), newDevice.swVersion()) ||
+            !isAnnotationsEqual(oldDevice.annotations(), newDevice.annotations())) {
             synchronized (this) {
                 devices.replace(newDevice.id(), oldDevice, newDevice);
@@ -203,7 +210,7 @@ public class SimpleDeviceStore
                 PortNumber number = portDescription.portNumber();
                 Port oldPort = ports.get(number);
                 // update description
-                descs.putPortDesc(number, portDescription);
+                descs.putPortDesc(portDescription);
                 Port newPort = composePort(device, number, descsMap);
                 events.add(oldPort == null ?
@@ -225,12 +232,14 @@ public class SimpleDeviceStore
         return new DeviceEvent(PORT_ADDED, device, newPort);
     }
-    // CHecks if the specified port requires update and if so, it replaces the
+    // Checks if the specified port requires update and if so, it replaces the
     // existing entry in the map and returns corresponding event.
     private DeviceEvent updatePort(Device device, Port oldPort,
                                    Port newPort,
                                    ConcurrentMap<PortNumber, Port> ports) {
-        if (oldPort.isEnabled() != newPort.isEnabled()) {
+        if (oldPort.isEnabled() != newPort.isEnabled() ||
+            !isAnnotationsEqual(oldPort.annotations(), newPort.annotations())) {
+
             ports.put(oldPort.number(), newPort);
             return new DeviceEvent(PORT_UPDATED, device, newPort);
         }
@@ -272,17 +281,17 @@ public class SimpleDeviceStore
         checkArgument(descsMap != null, DEVICE_NOT_FOUND, deviceId);
         DeviceDescriptions descs = descsMap.get(providerId);
+        // assuming all providers must to give DeviceDescription
         checkArgument(descs != null,
                 "Device description for Device ID %s from Provider %s was not found",
                 deviceId, providerId);
-        // TODO: implement multi-provider
         synchronized (this) {
             ConcurrentMap<PortNumber, Port> ports = getPortMap(deviceId);
             final PortNumber number = portDescription.portNumber();
             Port oldPort = ports.get(number);
             // update description
-            descs.putPortDesc(number, portDescription);
+            descs.putPortDesc(portDescription);
             Port newPort = composePort(device, number, descsMap);
             if (oldPort == null) {
                 return createPort(device, newPort, ports);
@@ -321,6 +330,26 @@ public class SimpleDeviceStore
         }
     }
+    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.
      *
@@ -336,46 +365,67 @@ public class SimpleDeviceStore
         ProviderId primary = pickPrimaryPID(providerDescs);
         DeviceDescriptions desc = providerDescs.get(primary);
+
+        // base
         Type type = desc.getDeviceDesc().type();
         String manufacturer = desc.getDeviceDesc().manufacturer();
         String hwVersion = desc.getDeviceDesc().hwVersion();
         String swVersion = desc.getDeviceDesc().swVersion();
         String serialNumber = desc.getDeviceDesc().serialNumber();
+        DefaultAnnotations annotations = DefaultAnnotations.builder().build();
+        annotations = merge(annotations, desc.getDeviceDesc().annotations());
         for (Entry<ProviderId, DeviceDescriptions> e : providerDescs.entrySet()) {
             if (e.getKey().equals(primary)) {
                 continue;
             }
-            // FIXME: implement attribute merging once we have K-V attributes
+            // TODO: should keep track of Description timestamp
+            // and only merge conflicting keys when timestamp is newer
+            // Currently assuming there will never be a key conflict between
+            // providers
+
+            // annotation merging. not so efficient, should revisit later
+            annotations = merge(annotations, e.getValue().getDeviceDesc().annotations());
         }
-        return new DefaultDevice(primary, deviceId , type, manufacturer, hwVersion, swVersion, serialNumber);
+        return new DefaultDevice(primary, deviceId , type, manufacturer,
+                            hwVersion, swVersion, serialNumber, annotations);
     }
-    // probably want composePorts
+    // probably want composePort"s" also
     private Port composePort(Device device, PortNumber number,
                 ConcurrentMap<ProviderId, DeviceDescriptions> providerDescs) {
         ProviderId primary = pickPrimaryPID(providerDescs);
         DeviceDescriptions primDescs = providerDescs.get(primary);
+        // if no primary, assume not enabled
+        // TODO: revisit this default port enabled/disabled behavior
+        boolean isEnabled = false;
+        DefaultAnnotations annotations = DefaultAnnotations.builder().build();
+
         final PortDescription portDesc = primDescs.getPortDesc(number);
-        boolean isEnabled;
         if (portDesc != null) {
             isEnabled = portDesc.isEnabled();
-        } else {
+            annotations = merge(annotations, portDesc.annotations());
-            // if no primary, assume not enabled
-            // TODO: revisit this port enabled/disabled behavior
-            isEnabled = false;
         }
         for (Entry<ProviderId, DeviceDescriptions> e : providerDescs.entrySet()) {
             if (e.getKey().equals(primary)) {
                 continue;
             }
-            // FIXME: implement attribute merging once we have K-V attributes
+            // TODO: should keep track of Description timestamp
+            // and only merge conflicting keys when timestamp is newer
+            // Currently assuming there will never be a key conflict between
+            // providers
+
+            // annotation merging. not so efficient, should revisit later
+            final PortDescription otherPortDesc = e.getValue().getPortDesc(number);
+            if (otherPortDesc != null) {
+                annotations = merge(annotations, otherPortDesc.annotations());
+            }
         }
-        return new DefaultPort(device, number, isEnabled);
+        return new DefaultPort(device, number, isEnabled, annotations);
     }
     /**
@@ -428,7 +478,7 @@ public class SimpleDeviceStore
         private final ConcurrentMap<PortNumber, PortDescription> portDescs;
         public DeviceDescriptions(DeviceDescription desc) {
-            this.deviceDesc = new AtomicReference<>(desc);
+            this.deviceDesc = new AtomicReference<>(checkNotNull(desc));
             this.portDescs = new ConcurrentHashMap<>();
         }
@@ -444,12 +494,38 @@ public class SimpleDeviceStore
             return Collections.unmodifiableCollection(portDescs.values());
         }
-        public DeviceDescription putDeviceDesc(DeviceDescription newDesc) {
+        /**
-            return deviceDesc.getAndSet(newDesc);
+         * Puts DeviceDescription, merging annotations as necessary.
+         *
+         * @param newDesc new DeviceDescription
+         * @return previous DeviceDescription
+         */
+        public synchronized DeviceDescription putDeviceDesc(DeviceDescription newDesc) {
+            DeviceDescription oldOne = deviceDesc.get();
+            DeviceDescription newOne = newDesc;
+            if (oldOne != null) {
+                SparseAnnotations merged = merge(oldOne.annotations(),
+                                                 newDesc.annotations());
+                newOne = new DefaultDeviceDescription(newOne, merged);
+            }
+            return deviceDesc.getAndSet(newOne);
         }
-        public PortDescription putPortDesc(PortNumber number, PortDescription newDesc) {
+        /**
-            return portDescs.put(number, newDesc);
+         * Puts PortDescription, merging annotations as necessary.
+         *
+         * @param newDesc new PortDescription
+         * @return previous PortDescription
+         */
+        public synchronized PortDescription putPortDesc(PortDescription newDesc) {
+            PortDescription oldOne = portDescs.get(newDesc.portNumber());
+            PortDescription newOne = newDesc;
+            if (oldOne != null) {
+                SparseAnnotations merged = merge(oldOne.annotations(),
+                                                 newDesc.annotations());
+                newOne = new DefaultPortDescription(newOne, merged);
+            }
+            return portDescs.put(newOne.portNumber(), newOne);
         }
     }
 }
--- a/core/store/trivial/src/test/java/org/onlab/onos/store/trivial/impl/SimpleDeviceStoreTest.java
View file @55710e7
+++ b/core/store/trivial/src/test/java/org/onlab/onos/store/trivial/impl/SimpleDeviceStoreTest.java
View file @55710e7
@@ -22,10 +22,13 @@ import org.junit.Before;
 import org.junit.BeforeClass;
 import org.junit.Ignore;
 import org.junit.Test;
+import org.onlab.onos.net.Annotations;
+import org.onlab.onos.net.DefaultAnnotations;
 import org.onlab.onos.net.Device;
 import org.onlab.onos.net.DeviceId;
 import org.onlab.onos.net.Port;
 import org.onlab.onos.net.PortNumber;
+import org.onlab.onos.net.SparseAnnotations;
 import org.onlab.onos.net.device.DefaultDeviceDescription;
 import org.onlab.onos.net.device.DefaultPortDescription;
 import org.onlab.onos.net.device.DeviceDescription;
@@ -57,6 +60,23 @@ public class SimpleDeviceStoreTest {
     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 SimpleDeviceStore simpleDeviceStore;
     private DeviceStore deviceStore;
@@ -106,6 +126,24 @@ public class SimpleDeviceStoreTest {
         assertEquals(SN, device.serialNumber());
     }
+    /**
+     * Verifies that Annotations created by merging {@code annotations} is
+     * equal to actual Annotations.
+     *
+     * @param actual Annotations to check
+     * @param annotations
+     */
+    private static void assertAnnotationsEquals(Annotations actual, SparseAnnotations... annotations) {
+        DefaultAnnotations expected = DefaultAnnotations.builder().build();
+        for (SparseAnnotations a : annotations) {
+            expected = DefaultAnnotations.merge(expected, a);
+        }
+        assertEquals(expected.keys(), actual.keys());
+        for (String key : expected.keys()) {
+            assertEquals(expected.value(key), actual.value(key));
+        }
+    }
+
     @Test
     public final void testGetDeviceCount() {
         assertEquals("initialy empty", 0, deviceStore.getDeviceCount());
@@ -171,26 +209,41 @@ public class SimpleDeviceStoreTest {
     public final void testCreateOrUpdateDeviceAncillary() {
         DeviceDescription description =
                 new DefaultDeviceDescription(DID1.uri(), SWITCH, MFR,
-                        HW, SW1, SN);
+                        HW, SW1, SN, A2);
         DeviceEvent event = deviceStore.createOrUpdateDevice(PIDA, DID1, description);
         assertEquals(DEVICE_ADDED, event.type());
         assertDevice(DID1, SW1, event.subject());
         assertEquals(PIDA, event.subject().providerId());
+        assertAnnotationsEquals(event.subject().annotations(), A2);
         assertFalse("Ancillary will not bring device up", deviceStore.isAvailable(DID1));
         DeviceDescription description2 =
                 new DefaultDeviceDescription(DID1.uri(), SWITCH, MFR,
-                        HW, SW2, SN);
+                        HW, SW2, SN, A1);
         DeviceEvent event2 = deviceStore.createOrUpdateDevice(PID, DID1, description2);
         assertEquals(DEVICE_UPDATED, event2.type());
         assertDevice(DID1, SW2, event2.subject());
         assertEquals(PID, event2.subject().providerId());
+        assertAnnotationsEquals(event2.subject().annotations(), A1, A2);
         assertTrue(deviceStore.isAvailable(DID1));
         assertNull("No change expected", deviceStore.createOrUpdateDevice(PID, DID1, description2));
         // For now, Ancillary is ignored once primary appears
         assertNull("No change expected", deviceStore.createOrUpdateDevice(PIDA, DID1, description));
+
+        // But, Ancillary annotations will be in effect
+        DeviceDescription description3 =
+                new DefaultDeviceDescription(DID1.uri(), SWITCH, MFR,
+                        HW, SW1, SN, A2_2);
+        DeviceEvent event3 = deviceStore.createOrUpdateDevice(PIDA, DID1, description3);
+        assertEquals(DEVICE_UPDATED, event3.type());
+        // basic information will be the one from Primary
+        assertDevice(DID1, SW2, event3.subject());
+        assertEquals(PID, event3.subject().providerId());
+        // but annotation from Ancillary will be merged
+        assertAnnotationsEquals(event3.subject().annotations(), A1, A2, A2_2);
+        assertTrue(deviceStore.isAvailable(DID1));
     }
@@ -299,27 +352,40 @@ public class SimpleDeviceStoreTest {
         putDeviceAncillary(DID1, SW1);
         putDevice(DID1, SW1);
         List<PortDescription> pds = Arrays.<PortDescription>asList(
-                new DefaultPortDescription(P1, true)
+                new DefaultPortDescription(P1, true, A1)
                 );
         deviceStore.updatePorts(PID, DID1, pds);
         DeviceEvent event = deviceStore.updatePortStatus(PID, DID1,
-                new DefaultPortDescription(P1, false));
+                new DefaultPortDescription(P1, false, A1_2));
         assertEquals(PORT_UPDATED, event.type());
         assertDevice(DID1, SW1, event.subject());
         assertEquals(P1, event.port().number());
+        assertAnnotationsEquals(event.port().annotations(), A1, A1_2);
         assertFalse("Port is disabled", event.port().isEnabled());
         DeviceEvent event2 = deviceStore.updatePortStatus(PIDA, DID1,
                 new DefaultPortDescription(P1, true));
         assertNull("Ancillary is ignored if primary exists", event2);
+        // but, Ancillary annotation update will be notified
         DeviceEvent event3 = deviceStore.updatePortStatus(PIDA, DID1,
-                new DefaultPortDescription(P2, true));
+                new DefaultPortDescription(P1, true, A2));
-        assertEquals(PORT_ADDED, event3.type());
+        assertEquals(PORT_UPDATED, event3.type());
         assertDevice(DID1, SW1, event3.subject());
-        assertEquals(P2, event3.port().number());
+        assertEquals(P1, event3.port().number());
-        assertFalse("Port is disabled if not given from provider", event3.port().isEnabled());
+        assertAnnotationsEquals(event3.port().annotations(), A1, A1_2, A2);
+        assertFalse("Port is disabled", event3.port().isEnabled());
+
+        // port only reported from Ancillary will be notified as down
+        DeviceEvent event4 = deviceStore.updatePortStatus(PIDA, DID1,
+                new DefaultPortDescription(P2, true));
+        assertEquals(PORT_ADDED, event4.type());
+        assertDevice(DID1, SW1, event4.subject());
+        assertEquals(P2, event4.port().number());
+        assertAnnotationsEquals(event4.port().annotations());
+        assertFalse("Port is disabled if not given from primary provider",
+                        event4.port().isEnabled());
     }
     @Test