Fixed a defect that allowed ancillary device providers to overwrite primary provider's data.

Thomas Vachuska
Commit 56dbeb18d35c026d849a2522a395ec8e22b4a153 56dbeb18 1 parent ae968a66
Showing 2 changed files with 60 additions and 52 deletions
core/store/dist/src/main/java/org/onlab/onos/store/device/impl/GossipDeviceStore.java
core/store/trivial/src/main/java/org/onlab/onos/store/trivial/impl/SimpleDeviceStore.java
--- a/core/store/dist/src/main/java/org/onlab/onos/store/device/impl/GossipDeviceStore.java
View file @56dbeb1
+++ b/core/store/dist/src/main/java/org/onlab/onos/store/device/impl/GossipDeviceStore.java
View file @56dbeb1
@@ -290,12 +290,17 @@ public class GossipDeviceStore
     private DeviceEvent updateDevice(ProviderId providerId,
                                      Device oldDevice,
                                      Device newDevice, Timestamp newTimestamp) {
-
         // We allow only certain attributes to trigger update
-        if (!Objects.equals(oldDevice.hwVersion(), newDevice.hwVersion()) ||
+        boolean propertiesChanged =
-            !Objects.equals(oldDevice.swVersion(), newDevice.swVersion()) ||
+                !Objects.equals(oldDevice.hwVersion(), newDevice.hwVersion()) ||
-            !AnnotationsUtil.isEqual(oldDevice.annotations(), newDevice.annotations())) {
+                        !Objects.equals(oldDevice.swVersion(), newDevice.swVersion());
-
+        boolean annotationsChanged =
+                !AnnotationsUtil.isEqual(oldDevice.annotations(), newDevice.annotations());
+
+        // Primary providers can respond to all changes, but ancillary ones
+        // should respond only to annotation changes.
+        if ((providerId.isAncillary() && annotationsChanged) ||
+                (!providerId.isAncillary() && (propertiesChanged || annotationsChanged))) {
             boolean replaced = devices.replace(newDevice.id(), oldDevice, newDevice);
             if (!replaced) {
                 verify(replaced,
--- a/core/store/trivial/src/main/java/org/onlab/onos/store/trivial/impl/SimpleDeviceStore.java
View file @56dbeb1
+++ b/core/store/trivial/src/main/java/org/onlab/onos/store/trivial/impl/SimpleDeviceStore.java
View file @56dbeb1
@@ -70,15 +70,16 @@ public class SimpleDeviceStore
     public static final String DEVICE_NOT_FOUND = "Device with ID %s not found";
-    // collection of Description given from various providers
+    // Collection of Description given from various providers
     private final ConcurrentMap<DeviceId, Map<ProviderId, DeviceDescriptions>>
-                                deviceDescs = Maps.newConcurrentMap();
+            deviceDescs = Maps.newConcurrentMap();
-    // cache of Device and Ports generated by compositing descriptions from providers
+    // Cache of Device and Ports generated by compositing descriptions from providers
     private final ConcurrentMap<DeviceId, Device> devices = Maps.newConcurrentMap();
-    private final ConcurrentMap<DeviceId, ConcurrentMap<PortNumber, Port>> devicePorts = Maps.newConcurrentMap();
+    private final ConcurrentMap<DeviceId, ConcurrentMap<PortNumber, Port>>
+            devicePorts = Maps.newConcurrentMap();
-    // available(=UP) devices
+    // Available (=UP) devices
     private final Set<DeviceId> availableDevices = Sets.newConcurrentHashSet();
@@ -113,19 +114,17 @@ public class SimpleDeviceStore
     @Override
     public DeviceEvent createOrUpdateDevice(ProviderId providerId,
-                                     DeviceId deviceId,
+                                            DeviceId deviceId,
-                                     DeviceDescription deviceDescription) {
+                                            DeviceDescription deviceDescription) {
-
         Map<ProviderId, DeviceDescriptions> providerDescs
-            = getOrCreateDeviceDescriptions(deviceId);
+                = getOrCreateDeviceDescriptions(deviceId);
         synchronized (providerDescs) {
             // locking per device
-
             DeviceDescriptions descs
-                = getOrCreateProviderDeviceDescriptions(providerDescs,
+                    = getOrCreateProviderDeviceDescriptions(providerDescs,
-                                                        providerId,
+                                                            providerId,
-                                                        deviceDescription);
+                                                            deviceDescription);
             Device oldDevice = devices.get(deviceId);
             // update description
@@ -145,12 +144,11 @@ public class SimpleDeviceStore
     // Creates the device and returns the appropriate event if necessary.
     // Guarded by deviceDescs value (=Device lock)
     private DeviceEvent createDevice(ProviderId providerId, Device newDevice) {
-
         // update composed device cache
         Device oldDevice = devices.putIfAbsent(newDevice.id(), newDevice);
         verify(oldDevice == null,
-                "Unexpected Device in cache. PID:%s [old=%s, new=%s]",
+               "Unexpected Device in cache. PID:%s [old=%s, new=%s]",
-                providerId, oldDevice, newDevice);
+               providerId, oldDevice, newDevice);
         if (!providerId.isAncillary()) {
             availableDevices.add(newDevice.id());
@@ -162,17 +160,24 @@ public class SimpleDeviceStore
     // Updates the device and returns the appropriate event if necessary.
     // Guarded by deviceDescs value (=Device lock)
     private DeviceEvent updateDevice(ProviderId providerId, Device oldDevice, Device newDevice) {
-
         // We allow only certain attributes to trigger update
-        if (!Objects.equals(oldDevice.hwVersion(), newDevice.hwVersion()) ||
+        boolean propertiesChanged =
-            !Objects.equals(oldDevice.swVersion(), newDevice.swVersion()) ||
+                !Objects.equals(oldDevice.hwVersion(), newDevice.hwVersion()) ||
-            !AnnotationsUtil.isEqual(oldDevice.annotations(), newDevice.annotations())) {
+                        !Objects.equals(oldDevice.swVersion(), newDevice.swVersion());
+        boolean annotationsChanged =
+                !AnnotationsUtil.isEqual(oldDevice.annotations(), newDevice.annotations());
+
+        // Primary providers can respond to all changes, but ancillary ones
+        // should respond only to annotation changes.
+        if ((providerId.isAncillary() && annotationsChanged) ||
+                (!providerId.isAncillary() && (propertiesChanged || annotationsChanged))) {
             boolean replaced = devices.replace(newDevice.id(), oldDevice, newDevice);
             if (!replaced) {
+                // FIXME: Is the enclosing if required here?
                 verify(replaced,
-                        "Replacing devices cache failed. PID:%s [expected:%s, found:%s, new=%s]",
+                       "Replacing devices cache failed. PID:%s [expected:%s, found:%s, new=%s]",
-                        providerId, oldDevice, devices.get(newDevice.id())
+                       providerId, oldDevice, devices.get(newDevice.id())
                         , newDevice);
             }
             if (!providerId.isAncillary()) {
@@ -193,7 +198,7 @@ public class SimpleDeviceStore
     @Override
     public DeviceEvent markOffline(DeviceId deviceId) {
         Map<ProviderId, DeviceDescriptions> providerDescs
-            = getOrCreateDeviceDescriptions(deviceId);
+                = getOrCreateDeviceDescriptions(deviceId);
         // locking device
         synchronized (providerDescs) {
@@ -212,9 +217,8 @@ public class SimpleDeviceStore
     @Override
     public List<DeviceEvent> updatePorts(ProviderId providerId,
-                                      DeviceId deviceId,
+                                         DeviceId deviceId,
-                                      List<PortDescription> portDescriptions) {
+                                         List<PortDescription> portDescriptions) {
-
         Device device = devices.get(deviceId);
         checkArgument(device != null, DEVICE_NOT_FOUND, deviceId);
@@ -226,8 +230,8 @@ public class SimpleDeviceStore
             DeviceDescriptions descs = descsMap.get(providerId);
             // every provider must provide DeviceDescription.
             checkArgument(descs != null,
-                    "Device description for Device ID %s from Provider %s was not found",
+                          "Device description for Device ID %s from Provider %s was not found",
-                    deviceId, providerId);
+                          deviceId, providerId);
             Map<PortNumber, Port> ports = getPortMap(deviceId);
@@ -247,8 +251,8 @@ public class SimpleDeviceStore
                 newPort = composePort(device, number, descsMap);
                 events.add(oldPort == null ?
-                        createPort(device, newPort, ports) :
+                                   createPort(device, newPort, ports) :
-                        updatePort(device, oldPort, newPort, ports));
+                                   updatePort(device, oldPort, newPort, ports));
             }
             events.addAll(pruneOldPorts(device, ports, processed));
@@ -272,7 +276,7 @@ public class SimpleDeviceStore
                                    Port newPort,
                                    Map<PortNumber, Port> ports) {
         if (oldPort.isEnabled() != newPort.isEnabled() ||
-            !AnnotationsUtil.isEqual(oldPort.annotations(), newPort.annotations())) {
+                !AnnotationsUtil.isEqual(oldPort.annotations(), newPort.annotations())) {
             ports.put(oldPort.number(), newPort);
             return new DeviceEvent(PORT_UPDATED, device, newPort);
@@ -303,7 +307,7 @@ public class SimpleDeviceStore
     // exist, it creates and registers a new one.
     private ConcurrentMap<PortNumber, Port> getPortMap(DeviceId deviceId) {
         return createIfAbsentUnchecked(devicePorts, deviceId,
-                NewConcurrentHashMap.<PortNumber, Port>ifNeeded());
+                                       NewConcurrentHashMap.<PortNumber, Port>ifNeeded());
     }
     private Map<ProviderId, DeviceDescriptions> getOrCreateDeviceDescriptions(
@@ -325,9 +329,8 @@ public class SimpleDeviceStore
     // Guarded by deviceDescs value (=Device lock)
     private DeviceDescriptions getOrCreateProviderDeviceDescriptions(
-                            Map<ProviderId, DeviceDescriptions> device,
+            Map<ProviderId, DeviceDescriptions> device,
-                            ProviderId providerId, DeviceDescription deltaDesc) {
+            ProviderId providerId, DeviceDescription deltaDesc) {
-
         synchronized (device) {
             DeviceDescriptions r = device.get(providerId);
             if (r == null) {
@@ -340,7 +343,7 @@ public class SimpleDeviceStore
     @Override
     public DeviceEvent updatePortStatus(ProviderId providerId, DeviceId deviceId,
-                                 PortDescription portDescription) {
+                                        PortDescription portDescription) {
         Device device = devices.get(deviceId);
         checkArgument(device != null, DEVICE_NOT_FOUND, deviceId);
@@ -351,8 +354,8 @@ public class SimpleDeviceStore
             DeviceDescriptions descs = descsMap.get(providerId);
             // assuming all providers must give DeviceDescription first
             checkArgument(descs != null,
-                    "Device description for Device ID %s from Provider %s was not found",
+                          "Device description for Device ID %s from Provider %s was not found",
-                    deviceId, providerId);
+                          deviceId, providerId);
             ConcurrentMap<PortNumber, Port> ports = getPortMap(deviceId);
             final PortNumber number = portDescription.portNumber();
@@ -404,19 +407,19 @@ public class SimpleDeviceStore
             availableDevices.remove(deviceId);
             descs.clear();
             return device == null ? null :
-                new DeviceEvent(DEVICE_REMOVED, device, null);
+                    new DeviceEvent(DEVICE_REMOVED, device, null);
         }
     }
     /**
      * Returns a Device, merging description given from multiple Providers.
      *
-     * @param deviceId device identifier
+     * @param deviceId      device identifier
      * @param providerDescs Collection of Descriptions from multiple providers
      * @return Device instance
      */
     private Device composeDevice(DeviceId deviceId,
-            Map<ProviderId, DeviceDescriptions> providerDescs) {
+                                 Map<ProviderId, DeviceDescriptions> providerDescs) {
         checkArgument(!providerDescs.isEmpty(), "No Device descriptions supplied");
@@ -447,21 +450,21 @@ public class SimpleDeviceStore
             annotations = merge(annotations, e.getValue().getDeviceDesc().annotations());
         }
-        return new DefaultDevice(primary, deviceId , type, manufacturer,
+        return new DefaultDevice(primary, deviceId, type, manufacturer,
-                            hwVersion, swVersion, serialNumber,
+                                 hwVersion, swVersion, serialNumber,
-                            chassisId, annotations);
+                                 chassisId, annotations);
     }
     /**
      * Returns a Port, merging description given from multiple Providers.
      *
-     * @param device device the port is on
+     * @param device   device the port is on
-     * @param number port number
+     * @param number   port number
      * @param descsMap Collection of Descriptions from multiple providers
      * @return Port instance
      */
     private Port composePort(Device device, PortNumber number,
-                Map<ProviderId, DeviceDescriptions> descsMap) {
+                             Map<ProviderId, DeviceDescriptions> descsMap) {
         ProviderId primary = pickPrimaryPID(descsMap);
         DeviceDescriptions primDescs = descsMap.get(primary);