implemented annotation merging on SimpleDeviceStore.
- Added annotation support to PortsDescriptions
Change-Id: I157e4fb93b8f387b405722b8d004501d993decda
diff --git a/core/api/src/main/java/org/onlab/onos/net/DefaultAnnotations.java b/core/api/src/main/java/org/onlab/onos/net/DefaultAnnotations.java
index 001518e..0c0f375 100644
--- a/core/api/src/main/java/org/onlab/onos/net/DefaultAnnotations.java
+++ b/core/api/src/main/java/org/onlab/onos/net/DefaultAnnotations.java
@@ -1,5 +1,6 @@
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 @@
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
diff --git a/core/api/src/main/java/org/onlab/onos/net/device/DefaultDeviceDescription.java b/core/api/src/main/java/org/onlab/onos/net/device/DefaultDeviceDescription.java
index fd04e5c..788d23a 100644
--- a/core/api/src/main/java/org/onlab/onos/net/device/DefaultDeviceDescription.java
+++ b/core/api/src/main/java/org/onlab/onos/net/device/DefaultDeviceDescription.java
@@ -45,6 +45,18 @@
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;
diff --git a/core/api/src/main/java/org/onlab/onos/net/device/DefaultPortDescription.java b/core/api/src/main/java/org/onlab/onos/net/device/DefaultPortDescription.java
index 1d52ac9..eb75ede 100644
--- a/core/api/src/main/java/org/onlab/onos/net/device/DefaultPortDescription.java
+++ b/core/api/src/main/java/org/onlab/onos/net/device/DefaultPortDescription.java
@@ -1,20 +1,43 @@
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;
diff --git a/core/api/src/main/java/org/onlab/onos/net/device/PortDescription.java b/core/api/src/main/java/org/onlab/onos/net/device/PortDescription.java
index f0dc8ee..f01b49c 100644
--- a/core/api/src/main/java/org/onlab/onos/net/device/PortDescription.java
+++ b/core/api/src/main/java/org/onlab/onos/net/device/PortDescription.java
@@ -1,11 +1,12 @@
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
diff --git a/core/store/trivial/src/main/java/org/onlab/onos/store/trivial/impl/SimpleDeviceStore.java b/core/store/trivial/src/main/java/org/onlab/onos/store/trivial/impl/SimpleDeviceStore.java
index bc0a055..0b0ae37 100644
--- a/core/store/trivial/src/main/java/org/onlab/onos/store/trivial/impl/SimpleDeviceStore.java
+++ b/core/store/trivial/src/main/java/org/onlab/onos/store/trivial/impl/SimpleDeviceStore.java
@@ -9,6 +9,8 @@
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.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 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 @@
= createIfAbsentUnchecked(providerDescs, providerId,
new InitDeviceDescs(deviceDescription));
+ // update description
descs.putDeviceDesc(deviceDescription);
-
Device newDevice = composeDevice(deviceId, providerDescs);
if (oldDevice == null) {
@@ -144,7 +150,8 @@
// 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 @@
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 @@
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 @@
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 @@
}
}
+ 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 @@
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 {
- // if no primary, assume not enabled
- // TODO: revisit this port enabled/disabled behavior
- isEnabled = false;
+ annotations = merge(annotations, portDesc.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
+ 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 @@
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 @@
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);
}
}
}
diff --git a/core/store/trivial/src/test/java/org/onlab/onos/store/trivial/impl/SimpleDeviceStoreTest.java b/core/store/trivial/src/test/java/org/onlab/onos/store/trivial/impl/SimpleDeviceStoreTest.java
index 431fba3..a0d6e1c 100644
--- a/core/store/trivial/src/test/java/org/onlab/onos/store/trivial/impl/SimpleDeviceStoreTest.java
+++ b/core/store/trivial/src/test/java/org/onlab/onos/store/trivial/impl/SimpleDeviceStoreTest.java
@@ -22,10 +22,13 @@
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 @@
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 @@
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 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 @@
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));
- assertEquals(PORT_ADDED, event3.type());
+ new DefaultPortDescription(P1, true, A2));
+ assertEquals(PORT_UPDATED, event3.type());
assertDevice(DID1, SW1, event3.subject());
- assertEquals(P2, event3.port().number());
- assertFalse("Port is disabled if not given from provider", event3.port().isEnabled());
+ assertEquals(P1, event3.port().number());
+ 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