Immutability for getMeters APIs
Change-Id: Iaf908766aa360e84e82306e398fff56c9593d8f1
diff --git a/core/api/src/main/java/org/onosproject/net/meter/MeterStore.java b/core/api/src/main/java/org/onosproject/net/meter/MeterStore.java
index 3f0efa0..675592a 100644
--- a/core/api/src/main/java/org/onosproject/net/meter/MeterStore.java
+++ b/core/api/src/main/java/org/onosproject/net/meter/MeterStore.java
@@ -70,8 +70,9 @@
* Updates a given meter's state with the provided state.
*
* @param meter a meter
+ * @return the updated meter
*/
- void updateMeterState(Meter meter);
+ Meter updateMeterState(Meter meter);
/**
* Obtains a meter matching the given meter key.
@@ -84,7 +85,7 @@
/**
* Returns all meters stored in the store.
*
- * @return a collection of meters
+ * @return an immutable copy of all meters
*/
Collection<Meter> getAllMeters();
@@ -93,7 +94,7 @@
* precise device.
*
* @param deviceId the device to get the meter list from
- * @return a collection of meters
+ * @return an immutable copy of the meters stored for a given device
*/
Collection<Meter> getAllMeters(DeviceId deviceId);
diff --git a/core/api/src/test/java/org/onosproject/store/service/TestConsistentMap.java b/core/api/src/test/java/org/onosproject/store/service/TestConsistentMap.java
index 84d1734..e0df8c6 100644
--- a/core/api/src/test/java/org/onosproject/store/service/TestConsistentMap.java
+++ b/core/api/src/test/java/org/onosproject/store/service/TestConsistentMap.java
@@ -44,6 +44,8 @@
private final String mapName;
private final AtomicLong counter = new AtomicLong(0);
private final Serializer serializer;
+ private Map<K, V> javaMap;
+
private TestConsistentMap(String mapName, Serializer serializer) {
map = new ConcurrentHashMap<>();
@@ -197,9 +199,7 @@
@Override
public Collection<Versioned<V>> values() {
- return map.values()
- .stream()
- .collect(Collectors.toList());
+ return map.values();
}
@Override
@@ -295,7 +295,12 @@
@Override
public Map<K, V> asJavaMap() {
- return new ConsistentMapBackedJavaMap<>(this);
+ synchronized (this) {
+ if (javaMap == null) {
+ javaMap = new ConsistentMapBackedJavaMap<>(this);
+ }
+ }
+ return javaMap;
}
public static Builder builder() {
diff --git a/core/net/src/main/java/org/onosproject/net/meter/impl/MeterManager.java b/core/net/src/main/java/org/onosproject/net/meter/impl/MeterManager.java
index d157260..a06a037 100644
--- a/core/net/src/main/java/org/onosproject/net/meter/impl/MeterManager.java
+++ b/core/net/src/main/java/org/onosproject/net/meter/impl/MeterManager.java
@@ -15,6 +15,7 @@
*/
package org.onosproject.net.meter.impl;
+import com.google.common.collect.Sets;
import org.onlab.util.PredictableExecutor;
import org.onlab.util.PredictableExecutor.PickyRunnable;
import org.onlab.util.Tools;
@@ -385,17 +386,26 @@
});
// Update the meter stats in the store (first time move the state from pending to added)
+ Collection<Meter> addedMeters = Sets.newHashSet();
meterEntries.stream()
.filter(m -> allMeters.stream()
.anyMatch(sm -> sm.deviceId().equals(deviceId) && sm.id().equals(m.id())))
- .forEach(m -> store.updateMeterState(m));
+ .forEach(m -> {
+ Meter updatedMeter = store.updateMeterState(m);
+ if (updatedMeter != null && updatedMeter.state() == MeterState.ADDED) {
+ addedMeters.add(updatedMeter);
+ }
+ });
+ Collection<Meter> newAllMeters = Sets.newHashSet(allMeters);
+ newAllMeters.removeAll(addedMeters);
- allMeters.forEach(m -> {
+ newAllMeters.forEach(m -> {
// FIXME: Installing a meter is meaningful for OpenFlow, but not for P4Runtime.
// It looks like this flow is used only for p4runtime to emulate the installation
// since meters are already instantiated - we need just modify the params.
if (m.state() == MeterState.PENDING_ADD && m.meterCellId().type() != MeterCellType.INDEX) {
// offload the task to avoid the overloading of the sb threads
+ log.debug("Modify meter {} in device {}", m.id(), deviceId);
meterInstallers.execute(new MeterInstaller(m.deviceId(), m, MeterOperation.Type.MODIFY));
// Remove workflow. Regarding OpenFlow, meters have been removed from
// the device but they are still in the store, we will purge them definitely.
@@ -403,6 +413,7 @@
// for P4Runtime will avoid to send a remove op. Then, we reach this point
// and we purge the meter from the store
} else if (m.state() == MeterState.PENDING_REMOVE) {
+ log.debug("Delete meter {} now in store", m.id());
store.deleteMeterNow(m);
}
});
diff --git a/core/store/dist/src/main/java/org/onosproject/store/meter/impl/DistributedMeterStore.java b/core/store/dist/src/main/java/org/onosproject/store/meter/impl/DistributedMeterStore.java
index 9aec726..11570f7 100644
--- a/core/store/dist/src/main/java/org/onosproject/store/meter/impl/DistributedMeterStore.java
+++ b/core/store/dist/src/main/java/org/onosproject/store/meter/impl/DistributedMeterStore.java
@@ -16,6 +16,7 @@
package org.onosproject.store.meter.impl;
import com.google.common.collect.Collections2;
+import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Iterables;
import com.google.common.collect.Lists;
import com.google.common.collect.Maps;
@@ -88,6 +89,7 @@
private static final String METERSTORE = "onos-meter-store";
private ConsistentMap<MeterKey, MeterData> meters;
private MapEventListener<MeterKey, MeterData> mapListener = new InternalMapEventListener();
+ private Map<MeterKey, MeterData> metersMap;
// Meters features related objects
private static final String METERFEATURESSTORE = "onos-meter-features-store";
@@ -157,6 +159,7 @@
Meter.Unit.class,
MeterFailReason.class,
MeterFeaturesFlag.class)).build();
+ metersMap = meters.asJavaMap();
// Init the set of the available ids
availableMeterIds = new DefaultDistributedSet<>(storageService.<MeterKey>setBuilder()
.withName(AVAILABLEMETERIDSTORE)
@@ -276,10 +279,10 @@
}
@Override
- public void updateMeterState(Meter meter) {
+ public Meter updateMeterState(Meter meter) {
// Update meter if present (stats workflow)
MeterKey key = MeterKey.key(meter.deviceId(), meter.id());
- meters.computeIfPresent(key, (k, v) -> {
+ Versioned<MeterData> value = meters.computeIfPresent(key, (k, v) -> {
DefaultMeter m = (DefaultMeter) v.meter();
MeterState meterState = m.state();
if (meterState == MeterState.PENDING_ADD) {
@@ -292,6 +295,7 @@
m.setReferenceCount(meter.referenceCount());
return new MeterData(m, null);
});
+ return value != null ? value.value().meter() : null;
}
@Override
@@ -302,14 +306,14 @@
@Override
public Collection<Meter> getAllMeters() {
- return Collections2.transform(meters.asJavaMap().values(),
+ return Collections2.transform(ImmutableSet.copyOf(metersMap.values()),
MeterData::meter);
}
@Override
public Collection<Meter> getAllMeters(DeviceId deviceId) {
return Collections2.transform(
- Collections2.filter(meters.asJavaMap().values(),
+ Collections2.filter(ImmutableSet.copyOf(metersMap.values()),
(MeterData m) -> m.meter().deviceId().equals(deviceId)),
MeterData::meter);
}
diff --git a/core/store/dist/src/test/java/org/onosproject/store/meter/impl/DistributedMeterStoreTest.java b/core/store/dist/src/test/java/org/onosproject/store/meter/impl/DistributedMeterStoreTest.java
index 2566803..4cddf54 100644
--- a/core/store/dist/src/test/java/org/onosproject/store/meter/impl/DistributedMeterStoreTest.java
+++ b/core/store/dist/src/test/java/org/onosproject/store/meter/impl/DistributedMeterStoreTest.java
@@ -45,6 +45,7 @@
import org.onosproject.store.service.Serializer;
import org.onosproject.store.service.TestStorageService;
+import java.util.Collection;
import java.util.Collections;
import java.util.HashSet;
import java.util.concurrent.CompletableFuture;
@@ -415,6 +416,64 @@
assertNull(meterStore.getMeter(keyOne));
}
+ /**
+ * Test getMeters API immutability.
+ */
+ @Test
+ public void testGetMetersImmutability() {
+ // Init the store
+ initMeterStore();
+
+ // Simulate the allocation of an id
+ MeterId idOne = meterStore.allocateMeterId(did1);
+ // Verify the allocation
+ assertThat(mid1, is(idOne));
+ // Let's create a meter
+ Meter meterOne = DefaultMeter.builder()
+ .forDevice(did1)
+ .fromApp(APP_ID)
+ .withId(mid1)
+ .withUnit(Meter.Unit.KB_PER_SEC)
+ .withBands(Collections.singletonList(b1))
+ .build();
+ // Set the state
+ ((DefaultMeter) meterOne).setState(MeterState.PENDING_ADD);
+ // Store the meter
+ meterStore.storeMeter(meterOne);
+
+ // Verify the immutability
+ Collection<Meter> meters = meterStore.getAllMeters();
+ Collection<Meter> metersDevice = meterStore.getAllMeters(did1);
+ assertThat(1, is(meters.size()));
+ assertThat(1, is(metersDevice.size()));
+
+ MeterId idTwo = meterStore.allocateMeterId(did1);
+ // Verify the allocation
+ assertThat(mid2, is(idTwo));
+ // Let's create a meter
+ Meter meterTwo = DefaultMeter.builder()
+ .forDevice(did1)
+ .fromApp(APP_ID)
+ .withId(mid2)
+ .withUnit(Meter.Unit.KB_PER_SEC)
+ .withBands(Collections.singletonList(b1))
+ .build();
+ // Set the state
+ ((DefaultMeter) meterTwo).setState(MeterState.PENDING_ADD);
+ // Store the meter
+ meterStore.storeMeter(meterTwo);
+
+ assertThat(1, is(meters.size()));
+ assertThat(1, is(metersDevice.size()));
+
+ meters = meterStore.getAllMeters();
+ metersDevice = meterStore.getAllMeters(did1);
+ assertThat(2, is(meters.size()));
+ assertThat(2, is(metersDevice.size()));
+
+
+ }
+
// Test class for driver service.
private class TestDriverService extends DriverServiceAdapter {
@Override