[SDFAB-542] Meters cleanup and leftovers v2
- Code clean up (unused code, unuseful comments)
- Remove deprecated internal APIs
- Prevent the ejection of the meter pollers
- Prevent the ejection of the mf pollers
- Fix unproper filter of device events
- Fix delete on store which updated existing meters with dummy value
- Fix NPE in TofinoMeterProgrammable caused by default config
- Update unit tests
Change-Id: Ib2767e3ab3cf146693e61b7e1890419c9743d521
(cherry picked from commit a770879a950d1cc985db1a659da701551700e886)
diff --git a/core/api/src/main/java/org/onosproject/net/meter/DefaultMeter.java b/core/api/src/main/java/org/onosproject/net/meter/DefaultMeter.java
index f3e29fe..ef8b3df 100644
--- a/core/api/src/main/java/org/onosproject/net/meter/DefaultMeter.java
+++ b/core/api/src/main/java/org/onosproject/net/meter/DefaultMeter.java
@@ -35,7 +35,6 @@
*/
public final class DefaultMeter extends AbstractAnnotated implements Meter, MeterEntry {
-
private final MeterCellId cellId;
private final Optional<ApplicationId> appId;
private final Unit unit;
diff --git a/core/api/src/main/java/org/onosproject/net/meter/MeterService.java b/core/api/src/main/java/org/onosproject/net/meter/MeterService.java
index 2d37f87..82bab2c 100644
--- a/core/api/src/main/java/org/onosproject/net/meter/MeterService.java
+++ b/core/api/src/main/java/org/onosproject/net/meter/MeterService.java
@@ -125,7 +125,7 @@
* @param deviceId device identifier
*/
default void purgeMeters(DeviceId deviceId) {
- //Default implementation does nothing
+ throw new UnsupportedOperationException("purgeMeters not implemented");
}
/**
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 4abf233..f15438b 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
@@ -36,16 +36,6 @@
CompletableFuture<MeterStoreResult> addOrUpdateMeter(Meter meter);
/**
- * Adds a meter to the store.
- *
- * @param meter a meter
- * @return a future indicating the result of the store operation
- * @deprecated in onos-2.5 replaced by {@link #addOrUpdateMeter(Meter)}
- */
- @Deprecated
- CompletableFuture<MeterStoreResult> storeMeter(Meter meter);
-
- /**
* Deletes a meter from the store.
*
* @param meter a meter
@@ -142,40 +132,10 @@
* Delete this meter immediately.
*
* @param m a meter
- * @deprecated in onos-2.5 renamed {@link #purgeMeter(Meter)}
- */
- @Deprecated
- void deleteMeterNow(Meter m);
-
- /**
- * Delete this meter immediately.
- *
- * @param m a meter
*/
void purgeMeter(Meter m);
/**
- * Retrieve maximum meters available for the device.
- *
- * @param key the meter features key
- * @return the maximum number of meters supported by the device
- * @deprecated in onos-2.5, Max meters is replaced by start and end index
- */
- @Deprecated
- long getMaxMeters(MeterFeaturesKey key);
-
- /**
- * Allocates the first available MeterId.
- *
- * @param deviceId the device id
- * @return the meter Id or null if it was not possible
- * to allocate a meter id
- * @deprecated in onos-2.5 replaced by {@link #allocateMeterId(DeviceId, MeterScope)}
- */
- @Deprecated
- MeterId allocateMeterId(DeviceId deviceId);
-
- /**
* Allocates the first available MeterId.
*
* @param deviceId the device id
@@ -201,16 +161,6 @@
* This API is typically used when the device is offline.
*
* @param deviceId the device id
- * @deprecated in onos-2.5, replaced by {@link #purgeMeters(DeviceId)}
- */
- @Deprecated
- void purgeMeter(DeviceId deviceId);
-
- /**
- * Removes all meters of given device from store.
- * This API is typically used when the device is offline.
- *
- * @param deviceId the device id
*/
void purgeMeters(DeviceId deviceId);
diff --git a/core/net/src/main/java/org/onosproject/net/meter/impl/MeterDriverProvider.java b/core/net/src/main/java/org/onosproject/net/meter/impl/MeterDriverProvider.java
index b1422e8..aad015f 100644
--- a/core/net/src/main/java/org/onosproject/net/meter/impl/MeterDriverProvider.java
+++ b/core/net/src/main/java/org/onosproject/net/meter/impl/MeterDriverProvider.java
@@ -23,8 +23,6 @@
import org.onosproject.net.device.DeviceEvent;
import org.onosproject.net.device.DeviceListener;
import org.onosproject.net.device.DeviceService;
-import org.onosproject.net.meter.Meter;
-import org.onosproject.net.meter.MeterFeatures;
import org.onosproject.net.meter.MeterOperation;
import org.onosproject.net.meter.MeterOperations;
import org.onosproject.net.meter.MeterProgrammable;
@@ -36,14 +34,11 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-import java.util.Collection;
import java.util.Collections;
import java.util.Set;
-import java.util.concurrent.ExecutionException;
import java.util.concurrent.ScheduledExecutorService;
import java.util.concurrent.ScheduledFuture;
import java.util.concurrent.TimeUnit;
-import java.util.concurrent.TimeoutException;
import static java.util.concurrent.Executors.newSingleThreadScheduledExecutor;
import static org.onlab.util.Tools.groupedThreads;
@@ -54,18 +49,14 @@
* Driver-based Meter provider.
*/
public class MeterDriverProvider extends AbstractProvider implements MeterProvider {
-
- private final Logger log = LoggerFactory.getLogger(getClass());
-
// To be extracted for reuse as we deal with other.
private static final String SCHEME = "default";
private static final String PROVIDER_NAME = "org.onosproject.provider.meter";
- // potentially positive device event
- private static final Set<DeviceEvent.Type> POSITIVE_DEVICE_EVENT =
- Sets.immutableEnumSet(DEVICE_ADDED,
- DEVICE_AVAILABILITY_CHANGED);
+ private static final Set<DeviceEvent.Type> POSITIVE_DEVICE_EVENT = Sets.immutableEnumSet(
+ DEVICE_ADDED, DEVICE_AVAILABILITY_CHANGED);
+ private final Logger log = LoggerFactory.getLogger(getClass());
protected DeviceService deviceService;
protected MastershipService mastershipService;
MeterProviderService meterProviderService;
@@ -117,12 +108,15 @@
}
private void pollMeters() {
- deviceService.getAvailableDevices().forEach(device -> {
- if (mastershipService.isLocalMaster(device.id()) &&
- device.is(MeterProgrammable.class)) {
- pollDeviceMeters(device.id());
- }
- });
+ try {
+ deviceService.getAvailableDevices().forEach(device -> {
+ if (mastershipService.isLocalMaster(device.id()) && device.is(MeterProgrammable.class)) {
+ pollDeviceMeters(device);
+ }
+ });
+ } catch (Exception e) {
+ log.warn("Exception thrown while polling meters", e);
+ }
}
@Override
@@ -138,41 +132,32 @@
}
}
- private void pollDeviceMeters(DeviceId deviceId) {
- Collection<Meter> meters = null;
+ private void pollDeviceMeters(Device device) {
try {
- meters = getMeterProgrammable(deviceId).getMeters().get(pollFrequency, TimeUnit.SECONDS);
- } catch (InterruptedException | ExecutionException | TimeoutException e) {
- log.warn("Unable to get the Meters from {}, error: {}", deviceId, e.getMessage());
- log.debug("Exception: ", e);
- }
- meterProviderService.pushMeterMetrics(deviceId, meters);
- }
-
- private void getMeterFeatures(DeviceId deviceId) {
- Collection<MeterFeatures> meterFeatures = Collections.emptySet();
- try {
- if (isMeterProgrammable(deviceId)) {
- meterFeatures = getMeterProgrammable(deviceId).getMeterFeatures().get(pollFrequency, TimeUnit.SECONDS);
- }
+ meterProviderService.pushMeterMetrics(device.id(), device.as(MeterProgrammable.class).getMeters()
+ .completeOnTimeout(Collections.emptySet(), pollFrequency, TimeUnit.SECONDS).get());
} catch (Exception e) {
- log.warn("Unable to get the Meter Features from {}, error: {}", deviceId, e.getMessage());
+ log.warn("Unable to get the Meters from {}, error: {}", device, e.getMessage());
log.debug("Exception: ", e);
}
- meterProviderService.pushMeterFeatures(deviceId, meterFeatures);
}
- private boolean isMeterProgrammable(DeviceId deviceId) {
- Device device = deviceService.getDevice(deviceId);
- return device.is(MeterProgrammable.class);
+ private void getMeterFeatures(Device device) {
+ try {
+ meterProviderService.pushMeterFeatures(device.id(), device.as(MeterProgrammable.class).getMeterFeatures()
+ .completeOnTimeout(Collections.emptySet(), pollFrequency, TimeUnit.SECONDS).get());
+ } catch (Exception e) {
+ log.warn("Unable to get the Meter Features from {}, error: {}", device.id(), e.getMessage());
+ log.debug("Exception: ", e);
+ }
}
private MeterProgrammable getMeterProgrammable(DeviceId deviceId) {
Device device = deviceService.getDevice(deviceId);
- if (device.is(MeterProgrammable.class)) {
+ if (device != null && device.is(MeterProgrammable.class)) {
return device.as(MeterProgrammable.class);
} else {
- log.debug("Device {} is not meter programmable", deviceId);
+ log.debug("Device {} is not meter programmable or does not exist", deviceId);
return null;
}
}
@@ -186,9 +171,7 @@
@Override
public boolean isRelevant(DeviceEvent event) {
- Device device = event.subject();
- return POSITIVE_DEVICE_EVENT.contains(event.type()) &&
- device.is(MeterProgrammable.class);
+ return event.subject().is(MeterProgrammable.class);
}
private void handleEvent(DeviceEvent event) {
@@ -196,7 +179,7 @@
switch (event.type()) {
case DEVICE_ADDED:
- getMeterFeatures(device.id());
+ getMeterFeatures(device);
break;
case DEVICE_REMOVED:
case DEVICE_SUSPENDED:
@@ -206,11 +189,11 @@
break;
}
- boolean isRelevant = mastershipService.isLocalMaster(device.id()) &&
- deviceService.isAvailable(device.id());
+ boolean isRelevant = POSITIVE_DEVICE_EVENT.contains(event.type()) &&
+ mastershipService.isLocalMaster(device.id()) && deviceService.isAvailable(device.id());
if (isRelevant) {
- pollDeviceMeters(device.id());
+ pollDeviceMeters(device);
}
}
}
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 98e79a9..94e33eb 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
@@ -106,11 +106,10 @@
public class MeterManager
extends AbstractListenerProviderRegistry<MeterEvent, MeterListener, MeterProvider, MeterProviderService>
implements MeterService, MeterProviderRegistry {
- // Installer related objects
+
private PredictableExecutor meterInstallers;
private static final String WORKER_PATTERN = "installer-%d";
private static final String GROUP_THREAD_NAME = "onos/meter";
- // Logging facility, meter store delegate and listener for device events.
private final Logger log = getLogger(getClass());
private final MeterStoreDelegate delegate = new InternalMeterStoreDelegate();
private final DeviceListener deviceListener = new InternalDeviceListener();
@@ -152,7 +151,6 @@
// Action triggered when the futures related to submit and withdrawal complete
private TriConsumer<MeterRequest, MeterStoreResult, Throwable> onComplete;
- // Meter provider reference
private final MeterDriverProvider defaultProvider = new MeterDriverProvider();
// Node id used to verify who is charge of the meter ops
@@ -298,7 +296,7 @@
// Meter installation logic (happy ending case)
// PENDING -> stats -> ADDED -> future completes
m.setState(MeterState.PENDING_ADD);
- store.storeMeter(m).whenComplete((result, error) ->
+ store.addOrUpdateMeter(m).whenComplete((result, error) ->
onComplete.accept(request, result, error));
return m;
}
@@ -324,7 +322,6 @@
DefaultMeter m = (DefaultMeter) mBuilder.build();
// Meter removal logic (happy ending case)
// PENDING -> stats -> removed from the map -> future completes
- // There is no transition to the REMOVED state
m.setState(MeterState.PENDING_REMOVE);
store.deleteMeter(m).whenComplete((result, error) ->
onComplete.accept(request, result, error));
@@ -359,7 +356,7 @@
@Override
public MeterId allocateMeterId(DeviceId deviceId) {
// We delegate directly to the store
- return store.allocateMeterId(deviceId);
+ return (MeterId) store.allocateMeterId(deviceId, MeterScope.globalScope());
}
private MeterCellId allocateMeterId(DeviceId deviceId, MeterScope scope) {
@@ -375,7 +372,7 @@
@Override
public void purgeMeters(DeviceId deviceId) {
// We delegate directly to the store
- store.purgeMeter(deviceId);
+ store.purgeMeters(deviceId);
}
@Override
@@ -404,7 +401,6 @@
@Override
public void pushMeterMetrics(DeviceId deviceId, Collection<Meter> meterEntries) {
- // Each update on the store is reflected on this collection
Collection<Meter> allMeters = store.getAllMeters(deviceId);
Map<MeterCellId, Meter> meterEntriesMap = meterEntries.stream()
@@ -448,12 +444,8 @@
Collection<Meter> newAllMeters = Sets.newHashSet(allMeters);
newAllMeters.removeAll(addedMeters);
+ // Remove definetely the remaining meters
newAllMeters.forEach(m -> {
- // Remove workflow. Regarding OpenFlow, meters have been removed from
- // the device but they are still in the store, we will purge them definitely.
- // Instead, P4Runtime devices will not remove the meter. The first workaround
- // for P4Runtime will avoid to send a remove op. Then, we reach this point
- // and we purge the meter from the store
if (m.state() == MeterState.PENDING_REMOVE) {
log.debug("Delete meter {} now in store", m.meterCellId());
store.purgeMeter(m);
@@ -566,7 +558,7 @@
if (purge) {
log.info("PurgeOnDisconnection is requested for device {}, " +
"removing meters", deviceId);
- store.purgeMeter(deviceId);
+ store.purgeMeters(deviceId);
}
}
break;
diff --git a/core/net/src/test/java/org/onosproject/net/meter/impl/MeterManagerTest.java b/core/net/src/test/java/org/onosproject/net/meter/impl/MeterManagerTest.java
index 1510e35..8e5e33b 100644
--- a/core/net/src/test/java/org/onosproject/net/meter/impl/MeterManagerTest.java
+++ b/core/net/src/test/java/org/onosproject/net/meter/impl/MeterManagerTest.java
@@ -68,7 +68,6 @@
import org.onosproject.net.meter.MeterProviderService;
import org.onosproject.net.meter.MeterRequest;
import org.onosproject.net.meter.MeterScope;
-import org.onosproject.net.meter.MeterService;
import org.onosproject.net.meter.MeterState;
import org.onosproject.net.pi.PiPipeconfServiceAdapter;
import org.onosproject.net.pi.model.PiMeterId;
@@ -109,14 +108,10 @@
*/
public class MeterManagerTest {
- // Test node id
private static final NodeId NID_LOCAL = new NodeId("local");
-
- // Test ip address
private static final IpAddress LOCALHOST = IpAddress.valueOf("127.0.0.1");
private static final ProviderId PID = new ProviderId("of", "foo");
-
private static final ProviderId PROGRAMMABLE_PROVIDER = new ProviderId("foo", "foo");
private static final DeviceId PROGRAMMABLE_DID = DeviceId.deviceId("test:002");
@@ -127,44 +122,23 @@
new DefaultDevice(PROGRAMMABLE_PROVIDER, PROGRAMMABLE_DID, Device.Type.SWITCH,
"", "", "", "", null, ANNOTATIONS);
- private MeterService service;
-
- // Test Driver service used during the tests
- private DriverManager driverService;
-
- // Test device service used during the tests
- private DeviceService deviceService;
-
- // Test provider used during the tests
private TestProvider provider;
-
- // Meter manager
private MeterManager manager;
-
- // Meter provider registry
private MeterProviderRegistry registry;
-
- // Meter provider service
private MeterProviderService providerService;
-
- // Store under testing
private DistributedMeterStore meterStore;
- // Device ids used during the tests
private DeviceId did1 = did("1");
private DeviceId did2 = did("2");
- // Meter ids used during the tests
private MeterId mid1 = MeterId.meterId(1);
private MeterCellId cid0 = PiMeterCellId.ofIndirect(PiMeterId.of("foo"), 0L);
- // Bands used during the tests
private static Band b1 = DefaultBand.builder()
.ofType(Band.Type.DROP)
.withRate(500)
.build();
- // Meters used during the tests
private Meter m1 = DefaultMeter.builder()
.forDevice(did1)
.fromApp(APP_ID)
@@ -179,7 +153,6 @@
.withUnit(Meter.Unit.KB_PER_SEC)
.withBands(Collections.singletonList(b1))
.build();
-
private static Meter mProgrammable = DefaultMeter.builder()
.forDevice(PROGRAMMABLE_DID)
.fromApp(APP_ID)
@@ -194,7 +167,6 @@
.withUnit(Meter.Unit.KB_PER_SEC)
.withBands(Collections.singletonList(b1))
.build();
-
private static Meter mUserDefined = DefaultMeter.builder()
.forDevice(PROGRAMMABLE_DID)
.fromApp(APP_ID)
@@ -203,7 +175,6 @@
.withBands(Collections.singletonList(b1))
.build();
- // Meter requests used during the tests
private MeterRequest.Builder m1Request = DefaultMeterRequest.builder()
.forDevice(did1)
.fromApp(APP_ID)
@@ -214,7 +185,6 @@
.fromApp(APP_ID)
.withUnit(Meter.Unit.KB_PER_SEC)
.withBands(Collections.singletonList(b1));
-
private MeterRequest.Builder mProgrammableRequest = DefaultMeterRequest.builder()
.forDevice(PROGRAMMABLE_DID)
.fromApp(APP_ID)
@@ -225,7 +195,6 @@
.fromApp(APP_ID)
.withUnit(Meter.Unit.KB_PER_SEC)
.withBands(Collections.singletonList(b1));
-
private MeterRequest.Builder userDefinedRequest = DefaultMeterRequest.builder()
.forDevice(PROGRAMMABLE_DID)
.fromApp(APP_ID)
@@ -234,7 +203,6 @@
.withScope(MeterScope.of("foo"))
.withIndex(0L);
- // Meter features used during the tests
private MeterFeatures mef1 = DefaultMeterFeatures.builder().forDevice(did1)
.withMaxMeters(3L)
.withBandTypes(new HashSet<>())
@@ -279,72 +247,53 @@
@Before
public void setup() {
- //Init step for the deviceService
- deviceService = new TestDeviceService();
- //Init step for the driver registry and driver service.
+ DeviceService deviceService = new TestDeviceService();
DriverRegistryManager driverRegistry = new DriverRegistryManager();
- driverService = new TestDriverManager(driverRegistry, deviceService, new NetworkConfigServiceAdapter());
+ DriverManager driverService = new TestDriverManager(driverRegistry, deviceService,
+ new NetworkConfigServiceAdapter());
driverRegistry.addDriver(new DefaultDriver("foo", ImmutableList.of(), "",
"", "",
ImmutableMap.of(MeterProgrammable.class,
TestMeterProgrammable.class, MeterQuery.class, TestMeterQuery.class),
ImmutableMap.of()));
-
- // Init step for the store
meterStore = new DistributedMeterStore();
- // Let's initialize some internal services of the store
TestUtils.setField(meterStore, "storageService", new TestStorageService());
TestUtils.setField(meterStore, "driverService", driverService);
-
- // Inject TestApplicationId into the DistributedMeterStore serializer
KryoNamespace.Builder testKryoBuilder = TestUtils.getField(meterStore, "APP_KRYO_BUILDER");
testKryoBuilder.register(TestApplicationId.class);
Serializer testSerializer = Serializer.using(Lists.newArrayList(testKryoBuilder.build()));
TestUtils.setField(meterStore, "serializer", testSerializer);
- // Activate the store
meterStore.activate();
- // Init step for the manager
+
manager = new MeterManager();
- // Let's initialize some internal services of the manager
TestUtils.setField(manager, "store", meterStore);
injectEventDispatcher(manager, new TestEventDispatcher());
manager.deviceService = deviceService;
manager.mastershipService = new TestMastershipService();
manager.cfgService = new ComponentConfigAdapter();
manager.clusterService = new TestClusterService();
- // Init the reference of the registry
registry = manager;
-
manager.driverService = driverService;
- // Activate the manager
Dictionary<String, Object> cfgDict = new Hashtable<>();
expect(componentContext.getProperties()).andReturn(cfgDict);
replay(componentContext);
manager.activate(componentContext);
- // Initialize the test provider
provider = new TestProvider(PID);
- // Register the provider against the manager
providerService = registry.register(provider);
- // Verify register
assertTrue("provider should be registered",
registry.getProviders().contains(provider.id()));
}
@After
public void tearDown() {
- // Unregister provider
registry.unregister(provider);
- // Verify unregister
assertFalse("provider should not be registered",
registry.getProviders().contains(provider.id()));
- // Deactivate manager
manager.deactivate();
- // Remove event dispatcher
injectEventDispatcher(manager, null);
- // Deactivate store
meterStore.deactivate();
}
@@ -407,24 +356,18 @@
*/
@Test
public void testAdd() {
- // Init store
initMeterStore();
- // Submit meter request
manager.submit(m1Request.add());
- // Verify add
+
assertEquals("The meter was not added", 1, manager.getAllMeters().size());
assertEquals("The meter was not added", 1, manager.getMeters(did1).size());
- // Get Meter
Meter installingMeter = manager.getMeter(did1, mid1);
- // Verify add of installingMeter and pending add state
assertThat(installingMeter, is(m1));
- // Verify pending add state
assertThat(installingMeter.state(), is(MeterState.PENDING_ADD));
- // Let's simulate a working data-plane
+
pushMetrics(MeterOperation.Type.ADD, installingMeter);
- // Get meter
+
Meter installedMeter = manager.getMeter(did1, mid1);
- // Verify installation
assertThat(installedMeter.state(), is(MeterState.ADDED));
assertEquals("The meter was not installed", 1, manager.getAllMeters().size());
assertEquals("The meter was not installed", 1, manager.getMeters(did1).size());
@@ -478,21 +421,16 @@
*/
@Test
public void testRemove() {
- // Init store
initMeterStore();
- // Submit meter request
manager.submit(m1Request.add());
- // Withdraw meter
manager.withdraw(m1Request.remove(), m1.id());
- // Get Meter
+
Meter withdrawingMeter = manager.getMeter(did1, mid1);
- // Verify withdrawing
assertThat(withdrawingMeter.state(), is(MeterState.PENDING_REMOVE));
assertEquals("The meter was not withdrawn", 1, manager.getAllMeters().size());
assertEquals("The meter was not withdrawn", 1, manager.getMeters(did1).size());
- // Let's simulate a working data-plane
+
pushMetrics(MeterOperation.Type.REMOVE, withdrawingMeter);
- // Verify withdrawn
assertNull(manager.getMeter(did1, mid1));
assertEquals("The meter was not removed", 0, manager.getAllMeters().size());
assertEquals("The meter was not removed", 0, manager.getMeters(did1).size());
@@ -525,32 +463,24 @@
*/
@Test
public void testAddMultipleDevice() {
- // Init store
initMeterStore();
- // Submit meter 1
manager.submit(m1Request.add());
- // Submit meter 2
manager.submit(m2Request.add());
- // Verify add
+
assertEquals("The meter was not added", 2, manager.getAllMeters().size());
assertEquals("The meter was not added", 1, manager.getMeters(did1).size());
assertEquals("The meter was not added", 1, manager.getMeters(did2).size());
- // Get Meters
Meter installingMeter1 = manager.getMeter(did1, mid1);
Meter installingMeter2 = manager.getMeter(did2, mid1);
- // Verify add of installingMeter
assertThat(installingMeter1, is(m1));
assertThat(installingMeter2, is(m2));
- // Verify pending add state
assertThat(installingMeter1.state(), is(MeterState.PENDING_ADD));
assertThat(installingMeter2.state(), is(MeterState.PENDING_ADD));
- // Let's simulate a working data-plane
+
pushMetrics(MeterOperation.Type.ADD, installingMeter1);
pushMetrics(MeterOperation.Type.ADD, installingMeter2);
- // Get meter
Meter installedMeter1 = manager.getMeter(did1, mid1);
Meter installedMeter2 = manager.getMeter(did2, mid1);
- // Verify installation
assertThat(installedMeter1.state(), is(MeterState.ADDED));
assertThat(installedMeter2.state(), is(MeterState.ADDED));
assertEquals("The meter was not installed", 2, manager.getAllMeters().size());
@@ -563,29 +493,22 @@
*/
@Test
public void testRemoveMultipleDevice() {
- // Init store
initMeterStore();
- // Submit meter 1
manager.submit(m1Request.add());
- // Submit meter 2
manager.submit(m2Request.add());
- // Withdraw meter
manager.withdraw(m1Request.remove(), m1.id());
- // Withdraw meter
manager.withdraw(m2Request.remove(), m2.id());
- // Get Meters
+
Meter withdrawingMeter1 = manager.getMeter(did1, mid1);
Meter withdrawingMeter2 = manager.getMeter(did2, mid1);
- // Verify withdrawing
assertThat(withdrawingMeter1.state(), is(MeterState.PENDING_REMOVE));
assertThat(withdrawingMeter2.state(), is(MeterState.PENDING_REMOVE));
assertEquals("The meter was not withdrawn", 2, manager.getAllMeters().size());
assertEquals("The meter was not withdrawn", 1, manager.getMeters(did1).size());
assertEquals("The meter was not withdrawn", 1, manager.getMeters(did2).size());
- // Let's simulate a working data-plane
+
pushMetrics(MeterOperation.Type.REMOVE, withdrawingMeter1);
pushMetrics(MeterOperation.Type.REMOVE, withdrawingMeter2);
- // Verify withdrawn
assertNull(manager.getMeter(did1, mid1));
assertNull(manager.getMeter(did2, mid1));
assertEquals("The meter was not removed", 0, manager.getAllMeters().size());
@@ -598,18 +521,15 @@
*/
@Test
public void testPurge() {
- // Init store
initMeterStore();
- // Submit meter request
manager.submit(m1Request.add());
- // Verify submit
+
Meter submittingMeter = manager.getMeter(did1, mid1);
assertThat(submittingMeter.state(), is(MeterState.PENDING_ADD));
assertEquals("The meter was not added", 1, manager.getAllMeters().size());
assertEquals("The meter was not added", 1, manager.getMeters(did1).size());
- // Purge the meters
+
manager.purgeMeters(did1);
- // Verify purge
assertNull(manager.getMeter(did1, mid1));
assertEquals("The meter was not purged", 0, manager.getAllMeters().size());
assertEquals("The meter was not purged", 0, manager.getMeters(did1).size());
@@ -620,9 +540,9 @@
*/
@Test
public void testAddFromMeterProgrammable() {
- // Init store
initMeterStore();
manager.submit(mProgrammableRequest.add());
+
TestTools.assertAfter(500, () -> {
assertEquals("The meter was not added", 1, manager.getAllMeters().size());
assertThat(manager.getMeter(PROGRAMMABLE_DID, MeterId.meterId(1)), is(mProgrammable));
@@ -634,10 +554,10 @@
*/
@Test
public void testAddBatchFromMeterProgrammable() {
- // Init store
initMeterStore();
List<MeterOperation> operations = ImmutableList.of(new MeterOperation(mProgrammable, MeterOperation.Type.ADD));
manager.defaultProvider().performMeterOperation(PROGRAMMABLE_DID, new MeterOperations(operations));
+
TestTools.assertAfter(500, () -> {
assertEquals("The meter was not added", 1, meterOperations.size());
assertEquals("Wrong Meter Operation", meterOperations.get(0).meter().id(), mProgrammable.id());
@@ -650,11 +570,11 @@
*/
@Test
public void testGetFromMeterProgrammable() {
- // Init store
initMeterStore();
MeterDriverProvider fallback = (MeterDriverProvider) manager.defaultProvider();
testAddFromMeterProgrammable();
fallback.init(manager.deviceService, fallback.meterProviderService, manager.mastershipService, 1);
+
TestTools.assertAfter(2000, () -> {
assertEquals("The meter was not added", 1, manager.getAllMeters().size());
Meter m = manager.getMeters(PROGRAMMABLE_DID).iterator().next();
@@ -745,7 +665,6 @@
});
}
- // Test cluster service
private final class TestClusterService extends ClusterServiceAdapter {
private ControllerNode local = new DefaultControllerNode(NID_LOCAL, LOCALHOST);
@@ -819,7 +738,7 @@
@Override
public CompletableFuture<Collection<Meter>> getMeters() {
- //ADD METER
+ // ADD METER
Collection<Meter> meters = Lists.newArrayList();
DefaultMeter mProgrammableAdded = (DefaultMeter) mProgrammable;
mProgrammableAdded.setState(MeterState.ADDED);
@@ -834,7 +753,6 @@
@Override
public CompletableFuture<Collection<MeterFeatures>> getMeterFeatures() {
- //Currently unused.
return CompletableFuture.completedFuture(Collections.emptySet());
}
}
@@ -847,16 +765,15 @@
@Override
public void performMeterOperation(DeviceId deviceId, MeterOperations meterOps) {
- //Currently unused.
+
}
@Override
public void performMeterOperation(DeviceId deviceId, MeterOperation meterOp) {
- //Currently unused
+
}
}
- // Test mastership service
private final class TestMastershipService extends MastershipServiceAdapter {
@Override
public NodeId getMasterFor(DeviceId deviceId) {
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 5b9252d..b94375c 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
@@ -37,7 +37,6 @@
import org.onosproject.net.meter.MeterFailReason;
import org.onosproject.net.meter.MeterFeatures;
import org.onosproject.net.meter.MeterFeaturesFlag;
-import org.onosproject.net.meter.MeterFeaturesKey;
import org.onosproject.net.meter.MeterId;
import org.onosproject.net.meter.MeterKey;
import org.onosproject.net.meter.MeterOperation;
@@ -115,13 +114,10 @@
// Meters id related objects
private static final String AVAILABLEMETERIDSTORE = "onos-meters-available-store";
- // Available meter identifiers
protected ConcurrentMap<MeterTableKey, DistributedSet<MeterKey>> availableMeterIds;
- // Atomic counter map for generation of new identifiers;
private static final String METERIDSTORE = "onos-meters-id-store";
private AtomicCounterMap<MeterTableKey> meterIdGenerators;
- // Serializer related objects
private static final KryoNamespace.Builder APP_KRYO_BUILDER = KryoNamespace.newBuilder()
.register(KryoNamespaces.API)
.register(MeterKey.class)
@@ -169,22 +165,20 @@
@Activate
public void activate() {
- // Init meters map and setup the map listener
meters = storageService.<MeterKey, MeterData>consistentMapBuilder()
.withName(METERSTORE)
.withSerializer(serializer).build();
meters.addListener(metersMapListener);
metersMap = meters.asJavaMap();
- // Init meter features map
+
metersFeatures = storageService.<MeterTableKey, MeterFeatures>eventuallyConsistentMapBuilder()
.withName(METERFEATURESSTORE)
.withTimestampProvider((key, features) -> new WallClockTimestamp())
.withSerializer(APP_KRYO_BUILDER).build();
metersFeatures.addListener(featuresMapListener);
- // Init the map of the available ids set
- // Set will be created when a new Meter Features is pushed to the store
+
availableMeterIds = new ConcurrentHashMap<>();
- // Init atomic map counters
+
meterIdGenerators = storageService.<MeterTableKey>atomicCounterMapBuilder()
.withName(METERIDSTORE)
.withSerializer(Serializer.using(KryoNamespaces.API,
@@ -207,14 +201,11 @@
@Override
public CompletableFuture<MeterStoreResult> addOrUpdateMeter(Meter meter) {
- // Verify integrity of the index
checkArgument(validIndex(meter), "Meter index is not valid");
CompletableFuture<MeterStoreResult> future = new CompletableFuture<>();
MeterKey key = MeterKey.key(meter.deviceId(), meter.meterCellId());
MeterData data = new MeterData(meter, null);
- // Store the future related to the operation
futures.put(key, future);
- // Check if the meter exists
try {
meters.compute(key, (k, v) -> data);
} catch (StorageException e) {
@@ -227,25 +218,25 @@
}
@Override
- public CompletableFuture<MeterStoreResult> storeMeter(Meter meter) {
- return addOrUpdateMeter(meter);
- }
-
- @Override
public CompletableFuture<MeterStoreResult> deleteMeter(Meter meter) {
- // Init steps
CompletableFuture<MeterStoreResult> future = new CompletableFuture<>();
MeterKey key = MeterKey.key(meter.deviceId(), meter.meterCellId());
- // Store the future related to the operation
futures.put(key, future);
- // Create the meter data
- MeterData data = new MeterData(meter, null);
// Update the state of the meter. It will be pruned by observing
// that it has been removed from the dataplane.
try {
- // If it does not exist in the system
- if (meters.computeIfPresent(key, (k, v) -> data) == null) {
- // Complete immediately
+ Versioned<MeterData> versionedData = meters.computeIfPresent(key, (k, v) -> {
+ DefaultMeter m = (DefaultMeter) v.meter();
+ MeterState meterState = m.state();
+ if (meterState == MeterState.PENDING_REMOVE) {
+ return v;
+ }
+ m.setState(meter.state());
+ return new MeterData(m, v.reason().isPresent() ? v.reason().get() : null);
+ });
+ // If it does not exist in the system, completes immediately
+ if (versionedData == null) {
+ futures.remove(key);
future.complete(MeterStoreResult.success());
}
} catch (StorageException e) {
@@ -254,7 +245,6 @@
futures.remove(key);
future.completeExceptionally(e);
}
- // Done, return the future
return future;
}
@@ -300,15 +290,12 @@
e.getMessage(), e);
result = MeterStoreResult.fail(TIMEOUT);
}
-
return result;
}
@Override
public MeterStoreResult deleteMeterFeatures(Collection<MeterFeatures> meterfeatures) {
- // These store operations is treated as one single operation
- // If one of them is failed, Fail is returned
- // But the failed operation will not block the rest.
+ // Same logic of storeMeterFeatures
MeterStoreResult result = MeterStoreResult.success();
for (MeterFeatures mf : meterfeatures) {
try {
@@ -387,19 +374,12 @@
}
@Override
- public void deleteMeterNow(Meter m) {
- // This method is renamed in onos-2.5
- purgeMeter(m);
- }
-
- @Override
public void purgeMeter(Meter m) {
- // Once we receive the ack from the sb
- // create the key and remove definitely the meter
+ // Once we receive the ack from the sb, create the key
+ // remove definitely the meter and free the id
MeterKey key = MeterKey.key(m.deviceId(), m.meterCellId());
try {
if (Versioned.valueOrNull(meters.remove(key)) != null) {
- // Free the id
MeterScope scope;
if (m.meterCellId().type() == PIPELINE_INDEPENDENT) {
PiMeterCellId piMeterCellId = (PiMeterCellId) m.meterCellId();
@@ -417,12 +397,6 @@
}
@Override
- public void purgeMeter(DeviceId deviceId) {
- // This method is renamed in onos-2.5
- purgeMeters(deviceId);
- }
-
- @Override
public void purgeMeters(DeviceId deviceId) {
List<Versioned<MeterData>> metersPendingRemove = meters.stream()
.filter(e -> Objects.equals(e.getKey().deviceId(), deviceId))
@@ -454,20 +428,13 @@
return userDefinedIndexMode;
}
- @Override
- public long getMaxMeters(MeterFeaturesKey key) {
- // Leverage the meter features to know the max id
- // Create a Meter Table key with FeaturesKey's device and global scope
- MeterTableKey meterTableKey = MeterTableKey.key(key.deviceId(), MeterScope.globalScope());
- return getMaxMeters(meterTableKey);
- }
-
- private long getMaxMeters(MeterTableKey key) {
- // Leverage the meter features to know the max id
+ protected long getMaxMeters(MeterTableKey key) {
MeterFeatures features = metersFeatures.get(key);
return features == null ? 0L : features.maxMeter();
}
+ // Validate index using the meter features, useful mainly
+ // when user defined index mode is enabled
private boolean validIndex(Meter meter) {
long index;
MeterTableKey key;
@@ -481,6 +448,7 @@
index = meterId.id();
key = MeterTableKey.key(meter.deviceId(), MeterScope.globalScope());
} else {
+ log.warn("Unable to validate index unsupported cell type {}", meter.meterCellId().type());
return false;
}
@@ -491,52 +459,44 @@
}
private long getStartIndex(MeterTableKey key) {
- // Leverage the meter features to know the start id
- // Since we are using index now
- // if there is no features related to the key
- // -1 is returned
MeterFeatures features = metersFeatures.get(key);
return features == null ? -1L : features.startIndex();
}
private long getEndIndex(MeterTableKey key) {
- // Leverage the meter features to know the max id
- // Since we are using index now
- // if there is no features related to the key
- // -1 is returned
MeterFeatures features = metersFeatures.get(key);
return features == null ? -1L : features.endIndex();
}
- // queryMaxMeters is implemented in FullMetersAvailable behaviour.
+ // queryMaxMeters is implemented in MeterQuery behaviour implementations.
private long queryMaxMeters(DeviceId device) {
- // Get driver handler for this device
DriverHandler handler = driverService.createHandler(device);
- // If creation failed or the device does not have this behavior
if (handler == null || !handler.hasBehaviour(MeterQuery.class)) {
- // We cannot know max meter
return 0L;
}
- // Get the behavior
+
+ // FIXME architecturally this is not right, we should fallback to this
+ // behavior in the providers. Once we do that we can remove this code.
MeterQuery query = handler.behaviour(MeterQuery.class);
- // Insert a new available key set to the map
+ // This results to be necessary because the available ids sets are created
+ // in the meter features map listener if the device does not provide the meter
+ // feature this is the only chance to create this set.
String setName = AVAILABLEMETERIDSTORE + "-" + device + "global";
MeterTableKey meterTableKey = MeterTableKey.key(device, MeterScope.globalScope());
insertAvailableKeySet(meterTableKey, setName);
- // Return as max meter the result of the query
+
return query.getMaxMeters();
}
private boolean updateMeterIdAvailability(MeterTableKey meterTableKey, MeterCellId id,
boolean available) {
- // Retrieve the set first
DistributedSet<MeterKey> keySet = availableMeterIds.get(meterTableKey);
if (keySet == null) {
- // A reusable set should be inserted when a features is pushed
log.warn("Reusable Key set for device: {} scope: {} not found",
meterTableKey.deviceId(), meterTableKey.scope());
return false;
}
+
// According to available, make available or unavailable a meter key
DeviceId deviceId = meterTableKey.deviceId();
return available ? keySet.add(MeterKey.key(deviceId, id)) :
@@ -544,62 +504,46 @@
}
private MeterCellId getNextAvailableId(Set<MeterCellId> availableIds) {
- // If there are no available ids
if (availableIds.isEmpty()) {
- // Just end the cycle
return null;
}
- // If it is the first fit
+
if (reuseStrategy == FIRST_FIT || availableIds.size() == 1) {
return availableIds.iterator().next();
}
- // If it is random, get the size
+
+ // If it is random, get the size and return a random element
int size = availableIds.size();
- // Return a random element
return Iterables.get(availableIds, RandomUtils.nextInt(size));
}
- // Implements reuse strategy
+ // Implements reuse strategy of the meter cell ids
private MeterCellId firstReusableMeterId(MeterTableKey meterTableKey) {
- // Create a Table key and use it to retrieve the reusable meterCellId set
DistributedSet<MeterKey> keySet = availableMeterIds.get(meterTableKey);
if (keySet == null) {
- // A reusable set should be inserted when a features is pushed
log.warn("Reusable Key set for device: {} scope: {} not found",
meterTableKey.deviceId(), meterTableKey.scope());
return null;
}
- // Filter key related to device id, and reduce to meter ids
+
Set<MeterCellId> localAvailableMeterIds = keySet.stream()
.filter(meterKey ->
meterKey.deviceId().equals(meterTableKey.deviceId()))
.map(MeterKey::meterCellId)
.collect(Collectors.toSet());
- // Get next available id
MeterCellId meterId = getNextAvailableId(localAvailableMeterIds);
- // Iterate until there are items
while (meterId != null) {
- // If we are able to reserve the id
if (updateMeterIdAvailability(meterTableKey, meterId, false)) {
- // Just end
return meterId;
}
- // Update the set
localAvailableMeterIds.remove(meterId);
- // Try another time
meterId = getNextAvailableId(localAvailableMeterIds);
}
- // No reusable ids
+ // there are no available ids that can be reused
return null;
}
@Override
- public MeterId allocateMeterId(DeviceId deviceId) {
- // We use global scope for MeterId
- return (MeterId) allocateMeterId(deviceId, MeterScope.globalScope());
- }
-
- @Override
public MeterCellId allocateMeterId(DeviceId deviceId, MeterScope meterScope) {
if (userDefinedIndexMode) {
log.warn("Unable to allocate meter id when user defined index mode is enabled");
@@ -611,19 +555,16 @@
// First, search for reusable key
meterCellId = firstReusableMeterId(meterTableKey);
if (meterCellId != null) {
- // A reusable key is found
return meterCellId;
}
// If there was no reusable meter id we have to generate a new value
// using start and end index as lower and upper bound respectively.
long startIndex = getStartIndex(meterTableKey);
long endIndex = getEndIndex(meterTableKey);
- // If the device does not give us MeterFeatures
+ // If the device does not give us MeterFeatures fallback to queryMeters
if (startIndex == -1L || endIndex == -1L) {
- // MeterFeatures couldn't be retrieved, fallback to queryMeters.
- // Only meaningful to OpenFLow
+ // Only meaningful for OpenFlow today
long maxMeters = queryMaxMeters(deviceId);
- // If we don't know the max, cannot proceed
if (maxMeters == 0L) {
return null;
} else {
@@ -632,18 +573,16 @@
endIndex = maxMeters;
}
}
- // Get a new value
- // If the value is smaller than the start index, get another one
+
do {
id = meterIdGenerators.getAndIncrement(meterTableKey);
} while (id < startIndex);
- // Check with the end index, and if the value is bigger, cannot proceed
if (id > endIndex) {
return null;
}
- // Done, return the value
- // If we are using global scope, return a MeterId
- // Else, return a PiMeterId
+
+ // For backward compatibility if we are using global scope,
+ // return a MeterId, otherwise we create a PiMeterCellId
if (meterScope.isGlobal()) {
return MeterId.meterId(id);
} else {
@@ -658,9 +597,23 @@
freeMeterId(meterTableKey, meterId);
}
- private void freeMeterId(MeterTableKey meterTableKey, MeterCellId meterCellId) {
+ protected void freeMeterId(DeviceId deviceId, MeterCellId meterCellId) {
+ MeterTableKey meterTableKey;
+ if (meterCellId.type() == PIPELINE_INDEPENDENT) {
+ meterTableKey = MeterTableKey.key(deviceId,
+ MeterScope.of(((PiMeterCellId) meterCellId).meterId().id()));
+ } else if (meterCellId.type() == INDEX) {
+ meterTableKey = MeterTableKey.key(deviceId, MeterScope.globalScope());
+ } else {
+ log.warn("Unable to free meter id unsupported cell type {}", meterCellId.type());
+ return;
+ }
+ freeMeterId(meterTableKey, meterCellId);
+ }
+
+ protected void freeMeterId(MeterTableKey meterTableKey, MeterCellId meterCellId) {
if (userDefinedIndexMode) {
- log.warn("Unable to free meter id when user defined index mode is enabled");
+ log.debug("Unable to free meter id when user defined index mode is enabled");
return;
}
long index;
@@ -671,13 +624,13 @@
MeterId meterId = (MeterId) meterCellId;
index = meterId.id();
} else {
+ log.warn("Unable to free meter id unsupported cell type {}", meterCellId.type());
return;
}
// Avoid to free meter not allocated
if (meterIdGenerators.get(meterTableKey) <= index) {
return;
}
- // Update the availability
updateMeterIdAvailability(meterTableKey, meterCellId, true);
}
@@ -728,12 +681,10 @@
}
break;
case REMOVE:
- // Meter removal case
futures.computeIfPresent(key, (k, v) -> {
v.complete(MeterStoreResult.success());
return null;
});
- // Finally notify the delegate
notifyDelegate(new MeterEvent(MeterEvent.Type.METER_REMOVED, data.meter()));
break;
default:
@@ -750,13 +701,11 @@
MeterFeatures meterFeatures = event.value();
switch (event.type()) {
case PUT:
- // Put a new available meter id set to the map
String setName = AVAILABLEMETERIDSTORE + "-" +
meterFeatures.deviceId() + meterFeatures.scope().id();
insertAvailableKeySet(meterTableKey, setName);
break;
case REMOVE:
- // Remove the set
DistributedSet<MeterKey> set = availableMeterIds.remove(meterTableKey);
if (set != null) {
set.destroy();
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 5268527..ee4d75c 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
@@ -37,7 +37,6 @@
import org.onosproject.net.meter.Meter;
import org.onosproject.net.meter.MeterCellId;
import org.onosproject.net.meter.MeterFeatures;
-import org.onosproject.net.meter.MeterFeaturesKey;
import org.onosproject.net.meter.MeterId;
import org.onosproject.net.meter.MeterKey;
import org.onosproject.net.meter.MeterScope;
@@ -64,32 +63,30 @@
* Meter store tests.
*/
public class DistributedMeterStoreTest {
- // Store under testing
+
private DistributedMeterStore meterStore;
- // Device ids used during the tests
private DeviceId did1 = did("1");
private DeviceId did2 = did("2");
private DeviceId did3 = did("3");
private DeviceId did4 = did("4");
- // Meter ids used during the tests
private MeterId mid1 = MeterId.meterId(1);
private MeterId mid2 = MeterId.meterId(2);
private MeterId mid3 = MeterId.meterId(3);
+ private MeterId mid5 = MeterId.meterId(5);
+ private MeterId mid6 = MeterId.meterId(6);
private MeterId mid10 = MeterId.meterId(10);
private MeterCellId cid4 = PiMeterCellId.ofIndirect(
PiMeterId.of("foo"), 4);
private MeterCellId invalidCid = PiMeterCellId.ofIndirect(
PiMeterId.of("foo"), 11);
- // Bands used during the tests
private Band b1 = DefaultBand.builder()
.ofType(Band.Type.DROP)
.withRate(500)
.build();
- // Meters used during the tests
private Meter m1 = DefaultMeter.builder()
.forDevice(did1)
.fromApp(APP_ID)
@@ -97,7 +94,6 @@
.withUnit(Meter.Unit.KB_PER_SEC)
.withBands(Collections.singletonList(b1))
.build();
-
private Meter m2 = DefaultMeter.builder()
.forDevice(did1)
.fromApp(APP_ID_2)
@@ -105,7 +101,6 @@
.withUnit(Meter.Unit.KB_PER_SEC)
.withBands(Collections.singletonList(b1))
.build();
-
private Meter m3 = DefaultMeter.builder()
.forDevice(did2)
.fromApp(APP_ID_2)
@@ -113,7 +108,6 @@
.withUnit(Meter.Unit.KB_PER_SEC)
.withBands(Collections.singletonList(b1))
.build();
-
private Meter m4 = DefaultMeter.builder()
.forDevice(did3)
.fromApp(APP_ID)
@@ -122,7 +116,6 @@
.withBands(Collections.singletonList(b1))
.build();
- // Meter features used during the tests
private MeterFeatures mef1 = DefaultMeterFeatures.builder().forDevice(did1)
.withMaxMeters(3L)
.withBandTypes(new HashSet<>())
@@ -155,33 +148,26 @@
@Before
public void setup() {
- // Init step
meterStore = new DistributedMeterStore();
- // Let's initialize some internal services
+
TestUtils.setField(meterStore, "storageService", new TestStorageService());
TestUtils.setField(meterStore, "driverService", new TestDriverService());
-
- // Inject TestApplicationId into the DistributedMeterStore serializer
KryoNamespace.Builder testKryoBuilder = TestUtils.getField(meterStore, "APP_KRYO_BUILDER");
testKryoBuilder.register(TestApplicationId.class);
Serializer testSerializer = Serializer.using(Lists.newArrayList(testKryoBuilder.build()));
TestUtils.setField(meterStore, "serializer", testSerializer);
- // Activate the store
meterStore.activate();
}
@After
public void tearDown() {
- // Deactivate the store
meterStore.deactivate();
}
private void initMeterStore(boolean enableUserDefinedIndex) {
meterStore.userDefinedIndexMode(enableUserDefinedIndex);
- // Let's store feature for device 1
meterStore.storeMeterFeatures(mef1);
- // Let's store feature for device 2
meterStore.storeMeterFeatures(mef2);
meterStore.storeMeterFeatures(mef3);
}
@@ -191,14 +177,10 @@
*/
@Test
public void testStoreMeterFeatures() {
- // Let's store feature for device 1
- meterStore.storeMeterFeatures(mef1);
- // Verify store meter features
- assertThat(meterStore.getMaxMeters(MeterFeaturesKey.key(did1)), is(3L));
- // Let's store feature for device 1
- meterStore.storeMeterFeatures(mef2);
- // Verify store meter features
- assertThat(meterStore.getMaxMeters(MeterFeaturesKey.key(did2)), is(10L));
+ initMeterStore(false);
+
+ assertThat(meterStore.getMaxMeters(MeterTableKey.key(did1, MeterScope.globalScope())), is(3L));
+ assertThat(meterStore.getMaxMeters(MeterTableKey.key(did2, MeterScope.globalScope())), is(10L));
}
/**
@@ -206,14 +188,11 @@
*/
@Test
public void testDeleteMeterFeatures() {
- // Let's store feature for device 1
- meterStore.storeMeterFeatures(mef1);
- // Verify store meter features
- assertThat(meterStore.getMaxMeters(MeterFeaturesKey.key(did1)), is(3L));
- // Let's delete the features
+ initMeterStore(false);
+ assertThat(meterStore.getMaxMeters(MeterTableKey.key(did1, MeterScope.globalScope())), is(3L));
+
meterStore.deleteMeterFeatures(did1);
- // Verify delete meter features
- assertThat(meterStore.getMaxMeters(MeterFeaturesKey.key(did1)), is(0L));
+ assertThat(meterStore.getMaxMeters(MeterTableKey.key(did1, MeterScope.globalScope())), is(0L));
}
/**
@@ -221,12 +200,10 @@
*/
@Test
public void testAllocateId() {
- // Init the store
initMeterStore(false);
- // Allocate a meter id and verify is equal to mid1
- assertThat(mid1, is(meterStore.allocateMeterId(did1)));
- // Allocate a meter id and verify is equal to mid2
- assertThat(mid2, is(meterStore.allocateMeterId(did1)));
+
+ assertThat(mid1, is(meterStore.allocateMeterId(did1, MeterScope.globalScope())));
+ assertThat(mid2, is(meterStore.allocateMeterId(did1, MeterScope.globalScope())));
}
/**
@@ -234,18 +211,16 @@
*/
@Test
public void testFreeId() {
- // Init the store
initMeterStore(false);
- // Allocate a meter id and verify is equal to mid1
- assertThat(mid1, is(meterStore.allocateMeterId(did1)));
- // Free the above id
+ assertThat(mid1, is(meterStore.allocateMeterId(did1, MeterScope.globalScope())));
+
+ // Verify reuse strategy
meterStore.freeMeterId(did1, mid1);
- // Allocate a meter id and verify is equal to mid1
- assertThat(mid1, is(meterStore.allocateMeterId(did1)));
- // Free an id not allocated
+ assertThat(mid1, is(meterStore.allocateMeterId(did1, MeterScope.globalScope())));
+
+ // Following free does not have effect
meterStore.freeMeterId(did1, mid10);
- // Allocate a meter id and verify is equal to mid2
- assertThat(mid2, is(meterStore.allocateMeterId(did1)));
+ assertThat(mid2, is(meterStore.allocateMeterId(did1, MeterScope.globalScope())));
}
/**
@@ -253,56 +228,39 @@
*/
@Test
public void testReuseId() {
- // Init the store
initMeterStore(false);
- // Reserve id 1
- MeterId meterIdOne = meterStore.allocateMeterId(did2);
- // Free the above id
+
+ MeterCellId meterIdOne = meterStore.allocateMeterId(did2, MeterScope.globalScope());
meterStore.freeMeterId(did2, meterIdOne);
- // Start an async reservation
- CompletableFuture<MeterId> future = CompletableFuture.supplyAsync(
- () -> meterStore.allocateMeterId(did2)
+ CompletableFuture<MeterCellId> future = CompletableFuture.supplyAsync(
+ () -> meterStore.allocateMeterId(did2, MeterScope.globalScope())
);
- // Start another reservation
- MeterId meterIdTwo = meterStore.allocateMeterId(did2);
+ MeterCellId meterIdTwo = meterStore.allocateMeterId(did2, MeterScope.globalScope());
try {
meterIdOne = future.get();
} catch (InterruptedException | ExecutionException e) {
e.printStackTrace();
}
- // Ids should be different, otherwise we had clash in the store
assertNotEquals("Ids should be different", meterIdOne, meterIdTwo);
- // Free the above id
meterStore.freeMeterId(did1, meterIdOne);
- // Free the above id
meterStore.freeMeterId(did1, meterIdTwo);
- // Reserve id 1
- meterIdOne = meterStore.allocateMeterId(did2);
- // Reserve id 2
- meterStore.allocateMeterId(did2);
- // Reserve id 3
- MeterId meterIdThree = meterStore.allocateMeterId(did2);
- // Reserve id 4
- MeterId meterIdFour = meterStore.allocateMeterId(did2);
- // Free the above id
+ meterIdOne = meterStore.allocateMeterId(did2, MeterScope.globalScope());
+ meterStore.allocateMeterId(did2, MeterScope.globalScope());
+ MeterCellId meterIdThree = meterStore.allocateMeterId(did2, MeterScope.globalScope());
+ MeterCellId meterIdFour = meterStore.allocateMeterId(did2, MeterScope.globalScope());
meterStore.freeMeterId(did1, meterIdOne);
- // Free the above id
meterStore.freeMeterId(did1, meterIdThree);
- // Free the above id
meterStore.freeMeterId(did1, meterIdFour);
- // Start an async reservation
future = CompletableFuture.supplyAsync(
- () -> meterStore.allocateMeterId(did2)
+ () -> meterStore.allocateMeterId(did2, MeterScope.globalScope())
);
- // Start another reservation
- MeterId meterAnotherId = meterStore.allocateMeterId(did2);
+ MeterCellId meterAnotherId = meterStore.allocateMeterId(did2, MeterScope.globalScope());
try {
meterAnotherId = future.get();
} catch (InterruptedException | ExecutionException e) {
e.printStackTrace();
}
- // Ids should be different, otherwise we had clash in the store
assertNotEquals("Ids should be different", meterAnotherId, meterIdOne);
}
@@ -311,12 +269,10 @@
*/
@Test
public void testQueryMeters() {
- // Init the store
initMeterStore(false);
- // Let's test queryMeters
- assertThat(mid1, is(meterStore.allocateMeterId(did3)));
- // Let's test queryMeters error
- assertNull(meterStore.allocateMeterId(did4));
+
+ assertThat(mid1, is(meterStore.allocateMeterId(did3, MeterScope.globalScope())));
+ assertNull(meterStore.allocateMeterId(did4, MeterScope.globalScope()));
}
/**
@@ -324,16 +280,12 @@
*/
@Test
public void testMaxMeterError() {
- // Init the store
initMeterStore(false);
- // Reserve id 1
- assertThat(mid1, is(meterStore.allocateMeterId(did1)));
- // Reserve id 2
- assertThat(mid2, is(meterStore.allocateMeterId(did1)));
- // Reserve id 3
- assertThat(mid3, is(meterStore.allocateMeterId(did1)));
- // Max meter error
- assertNull(meterStore.allocateMeterId(did1));
+
+ assertThat(mid1, is(meterStore.allocateMeterId(did1, MeterScope.globalScope())));
+ assertThat(mid2, is(meterStore.allocateMeterId(did1, MeterScope.globalScope())));
+ assertThat(mid3, is(meterStore.allocateMeterId(did1, MeterScope.globalScope())));
+ assertNull(meterStore.allocateMeterId(did1, MeterScope.globalScope()));
}
/**
@@ -341,13 +293,11 @@
*/
@Test
public void testStoreMeter() {
- // Init the store
initMeterStore(false);
- // Simulate the allocation of an id
- MeterId idOne = meterStore.allocateMeterId(did1);
- // Verify the allocation
+
+ MeterCellId idOne = meterStore.allocateMeterId(did1, MeterScope.globalScope());
assertThat(mid1, is(idOne));
- // Let's create a meter
+
Meter meterOne = DefaultMeter.builder()
.forDevice(did1)
.fromApp(APP_ID)
@@ -355,13 +305,9 @@
.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);
- // Let's create meter key
+ meterStore.addOrUpdateMeter(meterOne);
MeterKey meterKey = MeterKey.key(did1, mid1);
- // Verify the store
assertThat(1, is(meterStore.getAllMeters().size()));
assertThat(1, is(meterStore.getAllMeters(did1).size()));
assertThat(m1, is(meterStore.getMeter(meterKey)));
@@ -372,13 +318,11 @@
*/
@Test
public void testDeleteMeter() {
- // Init the store
initMeterStore(false);
- // Simulate the allocation of an id
- MeterId idOne = meterStore.allocateMeterId(did1);
- // Verify the allocation
+
+ MeterCellId idOne = meterStore.allocateMeterId(did1, MeterScope.globalScope());
assertThat(mid1, is(idOne));
- // Let's create a meter
+
Meter meterOne = DefaultMeter.builder()
.forDevice(did1)
.fromApp(APP_ID)
@@ -386,31 +330,23 @@
.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);
- // Set the state
+ meterStore.addOrUpdateMeter(meterOne);
((DefaultMeter) meterOne).setState(MeterState.PENDING_REMOVE);
- // Let's create meter key
MeterKey meterKey = MeterKey.key(did1, mid1);
- // Delete meter
meterStore.deleteMeter(meterOne);
- // Start an async delete, simulating the operation of the provider
CompletableFuture<Void> future = CompletableFuture.runAsync(
- () -> meterStore.deleteMeterNow(meterOne)
+ () -> meterStore.purgeMeter(meterOne)
);
- // Let's wait
try {
future.get();
} catch (InterruptedException | ExecutionException e) {
e.printStackTrace();
}
- // Verify delete
assertThat(0, is(meterStore.getAllMeters().size()));
assertThat(0, is(meterStore.getAllMeters(did1).size()));
assertNull(meterStore.getMeter(meterKey));
- assertThat(mid1, is(meterStore.allocateMeterId(did1)));
+ assertThat(mid1, is(meterStore.allocateMeterId(did1, MeterScope.globalScope())));
}
/**
@@ -418,13 +354,10 @@
*/
@Test
public void testNoDeleteMeter() {
- // Init the store
initMeterStore(false);
- // Simulate the allocation of an id
- MeterId idOne = meterStore.allocateMeterId(did1);
- // Create the key
+
+ MeterCellId idOne = meterStore.allocateMeterId(did1, MeterScope.globalScope());
MeterKey keyOne = MeterKey.key(did1, idOne);
- // Let's create a meter
Meter meterOne = DefaultMeter.builder()
.forDevice(did1)
.fromApp(APP_ID)
@@ -432,11 +365,8 @@
.withUnit(Meter.Unit.KB_PER_SEC)
.withBands(Collections.singletonList(b1))
.build();
- // Set the state
((DefaultMeter) meterOne).setState(MeterState.PENDING_REMOVE);
- // Delete meter
meterStore.deleteMeter(meterOne);
- // Verify No delete
assertThat(0, is(meterStore.getAllMeters().size()));
assertThat(0, is(meterStore.getAllMeters(did1).size()));
assertNull(meterStore.getMeter(keyOne));
@@ -446,11 +376,10 @@
* Test purge meter.
*/
@Test
- public void testPurgeMeter() {
- // add the meter
+ public void testPurgeMeters() {
testStoreMeter();
- meterStore.purgeMeter(did1);
- // Verify delete
+
+ meterStore.purgeMeters(did1);
MeterKey keyOne = MeterKey.key(did1, mid1);
assertThat(0, is(meterStore.getAllMeters().size()));
assertThat(0, is(meterStore.getAllMeters(did1).size()));
@@ -461,20 +390,18 @@
* Test purge meter given device and application.
*/
@Test
- public void testPurgeMeterDeviceAndApp() {
- // Init the store
+ public void testPurgeMetersDeviceAndApp() {
initMeterStore(false);
- // add the meters
+
((DefaultMeter) m1).setState(MeterState.PENDING_ADD);
((DefaultMeter) m2).setState(MeterState.PENDING_ADD);
((DefaultMeter) m3).setState(MeterState.PENDING_ADD);
- meterStore.storeMeter(m1);
- meterStore.storeMeter(m2);
- meterStore.storeMeter(m3);
+ meterStore.addOrUpdateMeter(m1);
+ meterStore.addOrUpdateMeter(m2);
+ meterStore.addOrUpdateMeter(m3);
assertThat(3, is(meterStore.getAllMeters().size()));
meterStore.purgeMeters(did1, APP_ID_2);
- // Verify delete
MeterKey keyTwo = MeterKey.key(did1, mid2);
assertThat(2, is(meterStore.getAllMeters().size()));
assertThat(1, is(meterStore.getAllMeters(did1).size()));
@@ -487,14 +414,10 @@
*/
@Test
public void testGetMetersImmutability() {
- // Init the store
initMeterStore(false);
- // Simulate the allocation of an id
- MeterId idOne = meterStore.allocateMeterId(did1);
- // Verify the allocation
+ MeterCellId idOne = meterStore.allocateMeterId(did1, MeterScope.globalScope());
assertThat(mid1, is(idOne));
- // Let's create a meter
Meter meterOne = DefaultMeter.builder()
.forDevice(did1)
.fromApp(APP_ID)
@@ -502,21 +425,16 @@
.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);
+ meterStore.addOrUpdateMeter(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
+ MeterCellId idTwo = meterStore.allocateMeterId(did1, MeterScope.globalScope());
assertThat(mid2, is(idTwo));
- // Let's create a meter
Meter meterTwo = DefaultMeter.builder()
.forDevice(did1)
.fromApp(APP_ID)
@@ -524,14 +442,11 @@
.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);
+ meterStore.addOrUpdateMeter(meterTwo);
assertThat(1, is(meters.size()));
assertThat(1, is(metersDevice.size()));
-
meters = meterStore.getAllMeters();
metersDevice = meterStore.getAllMeters(did1);
assertThat(2, is(meters.size()));
@@ -544,6 +459,7 @@
@Test(expected = IllegalArgumentException.class)
public void testInvalidCellId() {
initMeterStore(true);
+
// MF defines an end index equals to 10
Meter meterBad = DefaultMeter.builder()
.forDevice(did3)
@@ -553,7 +469,7 @@
.withBands(Collections.singletonList(b1))
.build();
((DefaultMeter) meterBad).setState(MeterState.PENDING_ADD);
- meterStore.storeMeter(meterBad);
+ meterStore.addOrUpdateMeter(meterBad);
}
/**
@@ -562,6 +478,7 @@
@Test
public void testEnableUserDefinedIndex() {
initMeterStore(false);
+
assertTrue(meterStore.userDefinedIndexMode(true));
}
@@ -571,6 +488,7 @@
@Test
public void testInvalidEnableUserDefinedIndex() {
testStoreMeter();
+
assertFalse(meterStore.userDefinedIndexMode(true));
}
@@ -580,6 +498,7 @@
@Test
public void testDisableUserDefinedIndex() {
initMeterStore(true);
+
assertFalse(meterStore.userDefinedIndexMode(false));
}
@@ -589,7 +508,7 @@
@Test
public void testStoreMeterInUserDefinedIndexMode() {
initMeterStore(true);
- // Let's create a meter
+
Meter meterOne = DefaultMeter.builder()
.forDevice(did3)
.fromApp(APP_ID)
@@ -597,13 +516,9 @@
.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);
- // Let's create meter key
+ meterStore.addOrUpdateMeter(meterOne);
MeterKey meterKey = MeterKey.key(did3, cid4);
- // Verify the store
assertThat(1, is(meterStore.getAllMeters().size()));
assertThat(1, is(meterStore.getAllMeters(did3).size()));
assertThat(m4, is(meterStore.getMeter(meterKey)));
@@ -615,6 +530,7 @@
@Test
public void testInvalidDisableUserDefinedIndex() {
testStoreMeterInUserDefinedIndexMode();
+
assertTrue(meterStore.userDefinedIndexMode(false));
}
@@ -624,7 +540,8 @@
@Test
public void testAllocateIdInUserDefinedIndexMode() {
initMeterStore(true);
- assertNull(meterStore.allocateMeterId(did1));
+
+ assertNull(meterStore.allocateMeterId(did1, MeterScope.globalScope()));
}
/**
@@ -633,7 +550,7 @@
@Test
public void testFreeIdInUserMode() {
initMeterStore(true);
- // Free the id and expect not being available
+
meterStore.freeMeterId(did1, mid1);
MeterTableKey globalKey = MeterTableKey.key(did1, MeterScope.globalScope());
assertNotNull(meterStore.availableMeterIds.get(globalKey));
@@ -646,6 +563,7 @@
@Test
public void testDeleteMeterInUserDefinedIndexMode() {
initMeterStore(true);
+
Meter meterOne = DefaultMeter.builder()
.forDevice(did3)
.fromApp(APP_ID)
@@ -654,7 +572,7 @@
.withBands(Collections.singletonList(b1))
.build();
((DefaultMeter) meterOne).setState(MeterState.PENDING_ADD);
- meterStore.storeMeter(meterOne);
+ meterStore.addOrUpdateMeter(meterOne);
((DefaultMeter) meterOne).setState(MeterState.PENDING_REMOVE);
MeterKey meterKey = MeterKey.key(did3, cid4);
@@ -676,7 +594,6 @@
assertTrue(meterStore.availableMeterIds.get(globalKey).isEmpty());
}
- // Test class for driver service.
private class TestDriverService extends DriverServiceAdapter {
@Override
public DriverHandler createHandler(DeviceId deviceId, String... credentials) {
@@ -684,7 +601,6 @@
}
}
- // Test class for driver handler.
private class TestDriverHandler implements DriverHandler {
@Override
@@ -714,7 +630,6 @@
}
}
- // Test meter query
private class TestMeterQuery implements MeterQuery {
@Override
diff --git a/drivers/barefoot/src/main/java/org/onosproject/drivers/barefoot/TofinoMeterProgrammable.java b/drivers/barefoot/src/main/java/org/onosproject/drivers/barefoot/TofinoMeterProgrammable.java
index 1a1ddd6..8d8c450 100644
--- a/drivers/barefoot/src/main/java/org/onosproject/drivers/barefoot/TofinoMeterProgrammable.java
+++ b/drivers/barefoot/src/main/java/org/onosproject/drivers/barefoot/TofinoMeterProgrammable.java
@@ -38,6 +38,13 @@
final PiMeterBand onosPeakBand = onosMeter.peakBand();
final PiMeterBand deviceCommittedBand = deviceMeter.committedBand();
final PiMeterBand devicePeakBand = deviceMeter.peakBand();
+
+ // Fail fast, this can easily happen if we send a write very
+ // close to a read, read can still return the default config
+ if (deviceCommittedBand == null || devicePeakBand == null) {
+ return false;
+ }
+
final long onosCir = onosCommittedBand.rate();
final long onosCburst = onosCommittedBand.burst();
final long onosPir = onosPeakBand.rate();
diff --git a/drivers/p4runtime/src/main/java/org/onosproject/drivers/p4runtime/P4RuntimeMeterProgrammable.java b/drivers/p4runtime/src/main/java/org/onosproject/drivers/p4runtime/P4RuntimeMeterProgrammable.java
index b903fa3..2230893 100644
--- a/drivers/p4runtime/src/main/java/org/onosproject/drivers/p4runtime/P4RuntimeMeterProgrammable.java
+++ b/drivers/p4runtime/src/main/java/org/onosproject/drivers/p4runtime/P4RuntimeMeterProgrammable.java
@@ -220,13 +220,14 @@
log.warn("Meter Cell Config obtained from device {} is different from " +
"one in in translation store: device={}, store=Default", deviceId, config);
} else {
- log.debug("Configs obtained from device: {} and present in the store are default, " +
- "skipping the forge section", deviceId);
+ log.debug("Configs for {} obtained from device: {} and from the store are default, " +
+ "skipping the forge section", config.cellId(), deviceId);
}
return null;
}
- // The config is not consistent
+ // The config is not consistent. MeterProgrammable should remember
+ // that config from devices can be default which means no band
if (!isSimilar(translatedEntity.get().translated(), config)) {
log.warn("Meter Cell Config obtained from device {} is different from " +
"one in in translation store: device={}, store={}",
@@ -243,7 +244,6 @@
Meter original = translatedEntity.get().original();
// Forge a meter with MeterCellId, Bands and DeviceId using the original value.
- // Other values are not required because we cannot retrieve them from the south
DefaultMeter meter = (DefaultMeter) DefaultMeter.builder()
.withBands(original.bands())
.withCellId(original.meterCellId())
diff --git a/web/api/src/main/java/org/onosproject/rest/resources/MetersWebResource.java b/web/api/src/main/java/org/onosproject/rest/resources/MetersWebResource.java
index f341bed..0f423a4 100644
--- a/web/api/src/main/java/org/onosproject/rest/resources/MetersWebResource.java
+++ b/web/api/src/main/java/org/onosproject/rest/resources/MetersWebResource.java
@@ -32,7 +32,6 @@
import org.onosproject.net.pi.model.PiMeterId;
import org.onosproject.net.pi.runtime.PiMeterCellId;
import org.onosproject.rest.AbstractWebResource;
-import org.slf4j.Logger;
import javax.ws.rs.Consumes;
import javax.ws.rs.DELETE;
@@ -51,7 +50,6 @@
import static org.onlab.util.Tools.nullIsNotFound;
import static org.onlab.util.Tools.readTreeFromStream;
-import static org.slf4j.LoggerFactory.getLogger;
/**
* Query and program meter rules.
@@ -62,7 +60,6 @@
@Context
private UriInfo uriInfo;
- private final Logger log = getLogger(getClass());
private static final String DEVICE_INVALID = "Invalid deviceId in meter creation request";
private static final String METER_NOT_FOUND = "Meter is not found for ";