Fixed remove behavior for Device and Link Store
Change-Id: I2d6c6a48f9b92136c2f0734d0216f9f3b05b4d8c
Showing
6 changed files
with
97 additions
and
35 deletions
... | @@ -33,6 +33,7 @@ import org.onlab.onos.store.AbstractStore; | ... | @@ -33,6 +33,7 @@ import org.onlab.onos.store.AbstractStore; |
33 | import org.onlab.onos.store.ClockService; | 33 | import org.onlab.onos.store.ClockService; |
34 | import org.onlab.onos.store.Timestamp; | 34 | import org.onlab.onos.store.Timestamp; |
35 | import org.onlab.onos.store.common.impl.Timestamped; | 35 | import org.onlab.onos.store.common.impl.Timestamped; |
36 | +import org.onlab.util.NewConcurrentHashMap; | ||
36 | import org.slf4j.Logger; | 37 | import org.slf4j.Logger; |
37 | 38 | ||
38 | import java.util.ArrayList; | 39 | import java.util.ArrayList; |
... | @@ -136,8 +137,7 @@ public class GossipDeviceStore | ... | @@ -136,8 +137,7 @@ public class GossipDeviceStore |
136 | 137 | ||
137 | // Collection of DeviceDescriptions for a Device | 138 | // Collection of DeviceDescriptions for a Device |
138 | ConcurrentMap<ProviderId, DeviceDescriptions> providerDescs | 139 | ConcurrentMap<ProviderId, DeviceDescriptions> providerDescs |
139 | - = createIfAbsentUnchecked(deviceDescs, deviceId, | 140 | + = getDeviceDescriptions(deviceId); |
140 | - new InitConcurrentHashMap<ProviderId, DeviceDescriptions>()); | ||
141 | 141 | ||
142 | 142 | ||
143 | DeviceDescriptions descs | 143 | DeviceDescriptions descs |
... | @@ -223,8 +223,7 @@ public class GossipDeviceStore | ... | @@ -223,8 +223,7 @@ public class GossipDeviceStore |
223 | @Override | 223 | @Override |
224 | public DeviceEvent markOffline(DeviceId deviceId) { | 224 | public DeviceEvent markOffline(DeviceId deviceId) { |
225 | ConcurrentMap<ProviderId, DeviceDescriptions> providerDescs | 225 | ConcurrentMap<ProviderId, DeviceDescriptions> providerDescs |
226 | - = createIfAbsentUnchecked(deviceDescs, deviceId, | 226 | + = getDeviceDescriptions(deviceId); |
227 | - new InitConcurrentHashMap<ProviderId, DeviceDescriptions>()); | ||
228 | 227 | ||
229 | // locking device | 228 | // locking device |
230 | synchronized (providerDescs) { | 229 | synchronized (providerDescs) { |
... | @@ -358,7 +357,13 @@ public class GossipDeviceStore | ... | @@ -358,7 +357,13 @@ public class GossipDeviceStore |
358 | // exist, it creates and registers a new one. | 357 | // exist, it creates and registers a new one. |
359 | private ConcurrentMap<PortNumber, Port> getPortMap(DeviceId deviceId) { | 358 | private ConcurrentMap<PortNumber, Port> getPortMap(DeviceId deviceId) { |
360 | return createIfAbsentUnchecked(devicePorts, deviceId, | 359 | return createIfAbsentUnchecked(devicePorts, deviceId, |
361 | - new InitConcurrentHashMap<PortNumber, Port>()); | 360 | + NewConcurrentHashMap.<PortNumber, Port>ifNeeded()); |
361 | + } | ||
362 | + | ||
363 | + private ConcurrentMap<ProviderId, DeviceDescriptions> getDeviceDescriptions( | ||
364 | + DeviceId deviceId) { | ||
365 | + return createIfAbsentUnchecked(deviceDescs, deviceId, | ||
366 | + NewConcurrentHashMap.<ProviderId, DeviceDescriptions>ifNeeded()); | ||
362 | } | 367 | } |
363 | 368 | ||
364 | @Override | 369 | @Override |
... | @@ -438,11 +443,17 @@ public class GossipDeviceStore | ... | @@ -438,11 +443,17 @@ public class GossipDeviceStore |
438 | 443 | ||
439 | @Override | 444 | @Override |
440 | public DeviceEvent removeDevice(DeviceId deviceId) { | 445 | public DeviceEvent removeDevice(DeviceId deviceId) { |
446 | + ConcurrentMap<ProviderId, DeviceDescriptions> descs = getDeviceDescriptions(deviceId); | ||
447 | + synchronized (descs) { | ||
441 | Device device = devices.remove(deviceId); | 448 | Device device = devices.remove(deviceId); |
442 | - // FIXME: should we be removing deviceDescs also? | 449 | + // should DEVICE_REMOVED carry removed ports? |
450 | + devicePorts.get(deviceId).clear(); | ||
451 | + availableDevices.remove(deviceId); | ||
452 | + descs.clear(); | ||
443 | return device == null ? null : | 453 | return device == null ? null : |
444 | new DeviceEvent(DEVICE_REMOVED, device, null); | 454 | new DeviceEvent(DEVICE_REMOVED, device, null); |
445 | } | 455 | } |
456 | + } | ||
446 | 457 | ||
447 | /** | 458 | /** |
448 | * Returns a Device, merging description given from multiple Providers. | 459 | * Returns a Device, merging description given from multiple Providers. |
... | @@ -546,14 +557,6 @@ public class GossipDeviceStore | ... | @@ -546,14 +557,6 @@ public class GossipDeviceStore |
546 | return fallBackPrimary; | 557 | return fallBackPrimary; |
547 | } | 558 | } |
548 | 559 | ||
549 | - private static final class InitConcurrentHashMap<K, V> implements | ||
550 | - ConcurrentInitializer<ConcurrentMap<K, V>> { | ||
551 | - @Override | ||
552 | - public ConcurrentMap<K, V> get() throws ConcurrentException { | ||
553 | - return new ConcurrentHashMap<>(); | ||
554 | - } | ||
555 | - } | ||
556 | - | ||
557 | public static final class InitDeviceDescs | 560 | public static final class InitDeviceDescs |
558 | implements ConcurrentInitializer<DeviceDescriptions> { | 561 | implements ConcurrentInitializer<DeviceDescriptions> { |
559 | 562 | ... | ... |
... | @@ -116,17 +116,19 @@ public class GossipDeviceStoreTest { | ... | @@ -116,17 +116,19 @@ public class GossipDeviceStoreTest { |
116 | deviceClockManager.deactivate(); | 116 | deviceClockManager.deactivate(); |
117 | } | 117 | } |
118 | 118 | ||
119 | - private void putDevice(DeviceId deviceId, String swVersion) { | 119 | + private void putDevice(DeviceId deviceId, String swVersion, |
120 | + SparseAnnotations... annotations) { | ||
120 | DeviceDescription description = | 121 | DeviceDescription description = |
121 | new DefaultDeviceDescription(deviceId.uri(), SWITCH, MFR, | 122 | new DefaultDeviceDescription(deviceId.uri(), SWITCH, MFR, |
122 | - HW, swVersion, SN); | 123 | + HW, swVersion, SN, annotations); |
123 | deviceStore.createOrUpdateDevice(PID, deviceId, description); | 124 | deviceStore.createOrUpdateDevice(PID, deviceId, description); |
124 | } | 125 | } |
125 | 126 | ||
126 | - private void putDeviceAncillary(DeviceId deviceId, String swVersion) { | 127 | + private void putDeviceAncillary(DeviceId deviceId, String swVersion, |
128 | + SparseAnnotations... annotations) { | ||
127 | DeviceDescription description = | 129 | DeviceDescription description = |
128 | new DefaultDeviceDescription(deviceId.uri(), SWITCH, MFR, | 130 | new DefaultDeviceDescription(deviceId.uri(), SWITCH, MFR, |
129 | - HW, swVersion, SN); | 131 | + HW, swVersion, SN, annotations); |
130 | deviceStore.createOrUpdateDevice(PIDA, deviceId, description); | 132 | deviceStore.createOrUpdateDevice(PIDA, deviceId, description); |
131 | } | 133 | } |
132 | 134 | ||
... | @@ -448,16 +450,37 @@ public class GossipDeviceStoreTest { | ... | @@ -448,16 +450,37 @@ public class GossipDeviceStoreTest { |
448 | 450 | ||
449 | @Test | 451 | @Test |
450 | public final void testRemoveDevice() { | 452 | public final void testRemoveDevice() { |
451 | - putDevice(DID1, SW1); | 453 | + putDevice(DID1, SW1, A1); |
454 | + List<PortDescription> pds = Arrays.<PortDescription>asList( | ||
455 | + new DefaultPortDescription(P1, true, A2) | ||
456 | + ); | ||
457 | + deviceStore.updatePorts(PID, DID1, pds); | ||
452 | putDevice(DID2, SW1); | 458 | putDevice(DID2, SW1); |
453 | 459 | ||
454 | assertEquals(2, deviceStore.getDeviceCount()); | 460 | assertEquals(2, deviceStore.getDeviceCount()); |
461 | + assertEquals(1, deviceStore.getPorts(DID1).size()); | ||
462 | + assertAnnotationsEquals(deviceStore.getDevice(DID1).annotations(), A1); | ||
463 | + assertAnnotationsEquals(deviceStore.getPort(DID1, P1).annotations(), A2); | ||
455 | 464 | ||
456 | DeviceEvent event = deviceStore.removeDevice(DID1); | 465 | DeviceEvent event = deviceStore.removeDevice(DID1); |
457 | assertEquals(DEVICE_REMOVED, event.type()); | 466 | assertEquals(DEVICE_REMOVED, event.type()); |
458 | assertDevice(DID1, SW1, event.subject()); | 467 | assertDevice(DID1, SW1, event.subject()); |
459 | 468 | ||
460 | assertEquals(1, deviceStore.getDeviceCount()); | 469 | assertEquals(1, deviceStore.getDeviceCount()); |
470 | + assertEquals(0, deviceStore.getPorts(DID1).size()); | ||
471 | + | ||
472 | + // putBack Device, Port w/o annotation | ||
473 | + putDevice(DID1, SW1); | ||
474 | + List<PortDescription> pds2 = Arrays.<PortDescription>asList( | ||
475 | + new DefaultPortDescription(P1, true) | ||
476 | + ); | ||
477 | + deviceStore.updatePorts(PID, DID1, pds2); | ||
478 | + | ||
479 | + // annotations should not survive | ||
480 | + assertEquals(2, deviceStore.getDeviceCount()); | ||
481 | + assertEquals(1, deviceStore.getPorts(DID1).size()); | ||
482 | + assertAnnotationsEquals(deviceStore.getDevice(DID1).annotations()); | ||
483 | + assertAnnotationsEquals(deviceStore.getPort(DID1, P1).annotations()); | ||
461 | } | 484 | } |
462 | 485 | ||
463 | // If Delegates should be called only on remote events, | 486 | // If Delegates should be called only on remote events, | ... | ... |
... | @@ -53,7 +53,6 @@ import static org.slf4j.LoggerFactory.getLogger; | ... | @@ -53,7 +53,6 @@ import static org.slf4j.LoggerFactory.getLogger; |
53 | import static org.apache.commons.lang3.concurrent.ConcurrentUtils.createIfAbsentUnchecked; | 53 | import static org.apache.commons.lang3.concurrent.ConcurrentUtils.createIfAbsentUnchecked; |
54 | import static org.onlab.onos.net.DefaultAnnotations.merge; | 54 | import static org.onlab.onos.net.DefaultAnnotations.merge; |
55 | 55 | ||
56 | -// TODO: synchronization should be done in more fine-grained manner. | ||
57 | /** | 56 | /** |
58 | * Manages inventory of infrastructure devices using trivial in-memory | 57 | * Manages inventory of infrastructure devices using trivial in-memory |
59 | * structures implementation. | 58 | * structures implementation. |
... | @@ -329,9 +328,16 @@ public class SimpleDeviceStore | ... | @@ -329,9 +328,16 @@ public class SimpleDeviceStore |
329 | 328 | ||
330 | @Override | 329 | @Override |
331 | public DeviceEvent removeDevice(DeviceId deviceId) { | 330 | public DeviceEvent removeDevice(DeviceId deviceId) { |
332 | - synchronized (this) { | 331 | + ConcurrentMap<ProviderId, DeviceDescriptions> descs = getDeviceDescriptions(deviceId); |
332 | + synchronized (descs) { | ||
333 | Device device = devices.remove(deviceId); | 333 | Device device = devices.remove(deviceId); |
334 | - // FIXME: should we be removing deviceDescs also? | 334 | + // should DEVICE_REMOVED carry removed ports? |
335 | + ConcurrentMap<PortNumber, Port> ports = devicePorts.get(deviceId); | ||
336 | + if (ports != null) { | ||
337 | + ports.clear(); | ||
338 | + } | ||
339 | + availableDevices.remove(deviceId); | ||
340 | + descs.clear(); | ||
335 | return device == null ? null : | 341 | return device == null ? null : |
336 | new DeviceEvent(DEVICE_REMOVED, device, null); | 342 | new DeviceEvent(DEVICE_REMOVED, device, null); |
337 | } | 343 | } | ... | ... |
... | @@ -47,7 +47,6 @@ import static org.slf4j.LoggerFactory.getLogger; | ... | @@ -47,7 +47,6 @@ import static org.slf4j.LoggerFactory.getLogger; |
47 | import static com.google.common.collect.Multimaps.synchronizedSetMultimap; | 47 | import static com.google.common.collect.Multimaps.synchronizedSetMultimap; |
48 | import static com.google.common.base.Predicates.notNull; | 48 | import static com.google.common.base.Predicates.notNull; |
49 | 49 | ||
50 | -// TODO: Add support for multiple provider and annotations | ||
51 | /** | 50 | /** |
52 | * Manages inventory of infrastructure links using trivial in-memory structures | 51 | * Manages inventory of infrastructure links using trivial in-memory structures |
53 | * implementation. | 52 | * implementation. |
... | @@ -230,7 +229,7 @@ public class SimpleLinkStore | ... | @@ -230,7 +229,7 @@ public class SimpleLinkStore |
230 | ConcurrentMap<ProviderId, LinkDescription> descs = getLinkDescriptions(key); | 229 | ConcurrentMap<ProviderId, LinkDescription> descs = getLinkDescriptions(key); |
231 | synchronized (descs) { | 230 | synchronized (descs) { |
232 | Link link = links.remove(key); | 231 | Link link = links.remove(key); |
233 | - // FIXME: should we be removing deviceDescs also? | 232 | + descs.clear(); |
234 | if (link != null) { | 233 | if (link != null) { |
235 | srcLinks.remove(link.src().deviceId(), key); | 234 | srcLinks.remove(link.src().deviceId(), key); |
236 | dstLinks.remove(link.dst().deviceId(), key); | 235 | dstLinks.remove(link.dst().deviceId(), key); | ... | ... |
... | @@ -103,17 +103,19 @@ public class SimpleDeviceStoreTest { | ... | @@ -103,17 +103,19 @@ public class SimpleDeviceStoreTest { |
103 | simpleDeviceStore.deactivate(); | 103 | simpleDeviceStore.deactivate(); |
104 | } | 104 | } |
105 | 105 | ||
106 | - private void putDevice(DeviceId deviceId, String swVersion) { | 106 | + private void putDevice(DeviceId deviceId, String swVersion, |
107 | + SparseAnnotations... annotations) { | ||
107 | DeviceDescription description = | 108 | DeviceDescription description = |
108 | new DefaultDeviceDescription(deviceId.uri(), SWITCH, MFR, | 109 | new DefaultDeviceDescription(deviceId.uri(), SWITCH, MFR, |
109 | - HW, swVersion, SN); | 110 | + HW, swVersion, SN, annotations); |
110 | deviceStore.createOrUpdateDevice(PID, deviceId, description); | 111 | deviceStore.createOrUpdateDevice(PID, deviceId, description); |
111 | } | 112 | } |
112 | 113 | ||
113 | - private void putDeviceAncillary(DeviceId deviceId, String swVersion) { | 114 | + private void putDeviceAncillary(DeviceId deviceId, String swVersion, |
115 | + SparseAnnotations... annotations) { | ||
114 | DeviceDescription description = | 116 | DeviceDescription description = |
115 | new DefaultDeviceDescription(deviceId.uri(), SWITCH, MFR, | 117 | new DefaultDeviceDescription(deviceId.uri(), SWITCH, MFR, |
116 | - HW, swVersion, SN); | 118 | + HW, swVersion, SN, annotations); |
117 | deviceStore.createOrUpdateDevice(PIDA, deviceId, description); | 119 | deviceStore.createOrUpdateDevice(PIDA, deviceId, description); |
118 | } | 120 | } |
119 | 121 | ||
... | @@ -437,16 +439,37 @@ public class SimpleDeviceStoreTest { | ... | @@ -437,16 +439,37 @@ public class SimpleDeviceStoreTest { |
437 | 439 | ||
438 | @Test | 440 | @Test |
439 | public final void testRemoveDevice() { | 441 | public final void testRemoveDevice() { |
440 | - putDevice(DID1, SW1); | 442 | + putDevice(DID1, SW1, A1); |
443 | + List<PortDescription> pds = Arrays.<PortDescription>asList( | ||
444 | + new DefaultPortDescription(P1, true, A2) | ||
445 | + ); | ||
446 | + deviceStore.updatePorts(PID, DID1, pds); | ||
441 | putDevice(DID2, SW1); | 447 | putDevice(DID2, SW1); |
442 | 448 | ||
443 | assertEquals(2, deviceStore.getDeviceCount()); | 449 | assertEquals(2, deviceStore.getDeviceCount()); |
450 | + assertEquals(1, deviceStore.getPorts(DID1).size()); | ||
451 | + assertAnnotationsEquals(deviceStore.getDevice(DID1).annotations(), A1); | ||
452 | + assertAnnotationsEquals(deviceStore.getPort(DID1, P1).annotations(), A2); | ||
444 | 453 | ||
445 | DeviceEvent event = deviceStore.removeDevice(DID1); | 454 | DeviceEvent event = deviceStore.removeDevice(DID1); |
446 | assertEquals(DEVICE_REMOVED, event.type()); | 455 | assertEquals(DEVICE_REMOVED, event.type()); |
447 | assertDevice(DID1, SW1, event.subject()); | 456 | assertDevice(DID1, SW1, event.subject()); |
448 | 457 | ||
449 | assertEquals(1, deviceStore.getDeviceCount()); | 458 | assertEquals(1, deviceStore.getDeviceCount()); |
459 | + assertEquals(0, deviceStore.getPorts(DID1).size()); | ||
460 | + | ||
461 | + // putBack Device, Port w/o annotation | ||
462 | + putDevice(DID1, SW1); | ||
463 | + List<PortDescription> pds2 = Arrays.<PortDescription>asList( | ||
464 | + new DefaultPortDescription(P1, true) | ||
465 | + ); | ||
466 | + deviceStore.updatePorts(PID, DID1, pds2); | ||
467 | + | ||
468 | + // annotations should not survive | ||
469 | + assertEquals(2, deviceStore.getDeviceCount()); | ||
470 | + assertEquals(1, deviceStore.getPorts(DID1).size()); | ||
471 | + assertAnnotationsEquals(deviceStore.getDevice(DID1).annotations()); | ||
472 | + assertAnnotationsEquals(deviceStore.getPort(DID1, P1).annotations()); | ||
450 | } | 473 | } |
451 | 474 | ||
452 | // If Delegates should be called only on remote events, | 475 | // If Delegates should be called only on remote events, | ... | ... |
... | @@ -91,16 +91,17 @@ public class SimpleLinkStoreTest { | ... | @@ -91,16 +91,17 @@ public class SimpleLinkStoreTest { |
91 | } | 91 | } |
92 | 92 | ||
93 | private void putLink(DeviceId srcId, PortNumber srcNum, | 93 | private void putLink(DeviceId srcId, PortNumber srcNum, |
94 | - DeviceId dstId, PortNumber dstNum, Type type) { | 94 | + DeviceId dstId, PortNumber dstNum, Type type, |
95 | + SparseAnnotations... annotations) { | ||
95 | ConnectPoint src = new ConnectPoint(srcId, srcNum); | 96 | ConnectPoint src = new ConnectPoint(srcId, srcNum); |
96 | ConnectPoint dst = new ConnectPoint(dstId, dstNum); | 97 | ConnectPoint dst = new ConnectPoint(dstId, dstNum); |
97 | - linkStore.createOrUpdateLink(PID, new DefaultLinkDescription(src, dst, type)); | 98 | + linkStore.createOrUpdateLink(PID, new DefaultLinkDescription(src, dst, type, annotations)); |
98 | } | 99 | } |
99 | 100 | ||
100 | - private void putLink(LinkKey key, Type type) { | 101 | + private void putLink(LinkKey key, Type type, SparseAnnotations... annotations) { |
101 | putLink(key.src().deviceId(), key.src().port(), | 102 | putLink(key.src().deviceId(), key.src().port(), |
102 | key.dst().deviceId(), key.dst().port(), | 103 | key.dst().deviceId(), key.dst().port(), |
103 | - type); | 104 | + type, annotations); |
104 | } | 105 | } |
105 | 106 | ||
106 | private static void assertLink(DeviceId srcId, PortNumber srcNum, | 107 | private static void assertLink(DeviceId srcId, PortNumber srcNum, |
... | @@ -351,8 +352,8 @@ public class SimpleLinkStoreTest { | ... | @@ -351,8 +352,8 @@ public class SimpleLinkStoreTest { |
351 | LinkKey linkId1 = new LinkKey(d1P1, d2P2); | 352 | LinkKey linkId1 = new LinkKey(d1P1, d2P2); |
352 | LinkKey linkId2 = new LinkKey(d2P2, d1P1); | 353 | LinkKey linkId2 = new LinkKey(d2P2, d1P1); |
353 | 354 | ||
354 | - putLink(linkId1, DIRECT); | 355 | + putLink(linkId1, DIRECT, A1); |
355 | - putLink(linkId2, DIRECT); | 356 | + putLink(linkId2, DIRECT, A2); |
356 | 357 | ||
357 | // DID1,P1 => DID2,P2 | 358 | // DID1,P1 => DID2,P2 |
358 | // DID2,P2 => DID1,P1 | 359 | // DID2,P2 => DID1,P1 |
... | @@ -360,10 +361,17 @@ public class SimpleLinkStoreTest { | ... | @@ -360,10 +361,17 @@ public class SimpleLinkStoreTest { |
360 | 361 | ||
361 | LinkEvent event = linkStore.removeLink(d1P1, d2P2); | 362 | LinkEvent event = linkStore.removeLink(d1P1, d2P2); |
362 | assertEquals(LINK_REMOVED, event.type()); | 363 | assertEquals(LINK_REMOVED, event.type()); |
364 | + assertAnnotationsEquals(event.subject().annotations(), A1); | ||
363 | LinkEvent event2 = linkStore.removeLink(d1P1, d2P2); | 365 | LinkEvent event2 = linkStore.removeLink(d1P1, d2P2); |
364 | assertNull(event2); | 366 | assertNull(event2); |
365 | 367 | ||
366 | assertLink(linkId2, DIRECT, linkStore.getLink(d2P2, d1P1)); | 368 | assertLink(linkId2, DIRECT, linkStore.getLink(d2P2, d1P1)); |
369 | + assertAnnotationsEquals(linkStore.getLink(d2P2, d1P1).annotations(), A2); | ||
370 | + | ||
371 | + // annotations, etc. should not survive remove | ||
372 | + putLink(linkId1, DIRECT); | ||
373 | + assertLink(linkId1, DIRECT, linkStore.getLink(d1P1, d2P2)); | ||
374 | + assertAnnotationsEquals(linkStore.getLink(d1P1, d2P2).annotations()); | ||
367 | } | 375 | } |
368 | 376 | ||
369 | @Test | 377 | @Test | ... | ... |
-
Please register or login to post a comment