[SDFAB-502] Improve P4RuntimeMeterProgrammable reconciliation
This is achieved by implementing device specific methods to verify if
ONOS store meters and values read from the devices are similar
Change-Id: I95b6a2c728536f08b47ce9d0d30d1b8888a353d7
diff --git a/drivers/barefoot/BUILD b/drivers/barefoot/BUILD
index ee63540..1633a15f 100644
--- a/drivers/barefoot/BUILD
+++ b/drivers/barefoot/BUILD
@@ -2,13 +2,16 @@
"//drivers/p4runtime:onos-drivers-p4runtime",
]
+TEST_DEPS = TEST_ADAPTERS
+
BUNDLES = [
":onos-drivers-barefoot",
]
-osgi_jar(
+osgi_jar_with_tests(
resources = glob(["src/main/resources/**"]),
resources_root = "src/main/resources",
+ test_deps = TEST_DEPS,
deps = COMPILE_DEPS,
)
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
new file mode 100644
index 0000000..1a1ddd6
--- /dev/null
+++ b/drivers/barefoot/src/main/java/org/onosproject/drivers/barefoot/TofinoMeterProgrammable.java
@@ -0,0 +1,81 @@
+/*
+ * Copyright 2021-present Open Networking Foundation
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.onosproject.drivers.barefoot;
+
+import org.onosproject.drivers.p4runtime.P4RuntimeMeterProgrammable;
+import org.onosproject.net.meter.MeterProgrammable;
+import org.onosproject.net.pi.runtime.PiMeterBand;
+import org.onosproject.net.pi.runtime.PiMeterCellConfig;
+
+/**
+ * Implementation of the MeterProgrammable behaviour for a Tofino-based switch.
+ */
+public class TofinoMeterProgrammable extends P4RuntimeMeterProgrammable implements MeterProgrammable {
+ // More insights for these magic numbers can be found in this doc
+ // https://docs.google.com/document/d/1Vuf_2RaO0BJPTj_weE9h2spUBCMsiBX-iIMb4OoEIB0/edit?usp=sharing
+ private static final double RATE_ERROR = 0.02;
+ private static final long BURST_LOWER_DIVIDER = 126;
+ private static final long BURST_UPPER_DIVIDER = 125;
+ private static final long BURST_MULTIPLIER = 125;
+
+ @Override
+ public boolean isSimilar(PiMeterCellConfig onosMeter, PiMeterCellConfig deviceMeter) {
+ final PiMeterBand onosCommittedBand = onosMeter.committedBand();
+ final PiMeterBand onosPeakBand = onosMeter.peakBand();
+ final PiMeterBand deviceCommittedBand = deviceMeter.committedBand();
+ final PiMeterBand devicePeakBand = deviceMeter.peakBand();
+ final long onosCir = onosCommittedBand.rate();
+ final long onosCburst = onosCommittedBand.burst();
+ final long onosPir = onosPeakBand.rate();
+ final long onosPburst = onosPeakBand.burst();
+ final long deviceCir = deviceCommittedBand.rate();
+ final long deviceCburst = deviceCommittedBand.burst();
+ final long devicePir = devicePeakBand.rate();
+ final long devicePburst = devicePeakBand.burst();
+
+ return isRateSimilar(onosCir, deviceCir) && isRateSimilar(onosPir, devicePir) &&
+ isBurstSimilar(onosCburst, deviceCburst) && isBurstSimilar(onosPburst, devicePburst);
+ }
+
+ // Verify if device rate is included in the confidence interval
+ // derived from the rate stored in ONOS
+ private boolean isRateSimilar(long onosRate, long deviceRate) {
+ double lowerEnd = (double) onosRate * (1.0 - RATE_ERROR);
+ double upperEnd = (double) onosRate * (1.0 + RATE_ERROR);
+
+ if (log.isDebugEnabled()) {
+ log.debug("isRateSimilar {} in [{}, {}]", deviceRate, lowerEnd, upperEnd);
+ }
+
+ return deviceRate >= lowerEnd && deviceRate <= upperEnd;
+ }
+
+ // Verify if device burst is included in the confidence interval
+ // derived from the burst stored in ONOS
+ private boolean isBurstSimilar(long onosBurst, long deviceBurst) {
+ // Rundown removing the decimal part
+ long lowerEnd = (onosBurst / BURST_LOWER_DIVIDER) * BURST_MULTIPLIER;
+ long upperEnd = (onosBurst / BURST_UPPER_DIVIDER) * BURST_MULTIPLIER;
+
+ if (log.isDebugEnabled()) {
+ log.debug("isBurstSimilar {} in [{}, {}]", deviceBurst, lowerEnd, upperEnd);
+ }
+
+ return deviceBurst >= lowerEnd && deviceBurst <= upperEnd;
+ }
+
+}
diff --git a/drivers/barefoot/src/main/resources/barefoot-drivers.xml b/drivers/barefoot/src/main/resources/barefoot-drivers.xml
index 89260f5..3b85dbe 100644
--- a/drivers/barefoot/src/main/resources/barefoot-drivers.xml
+++ b/drivers/barefoot/src/main/resources/barefoot-drivers.xml
@@ -27,6 +27,8 @@
hwVersion="Tofino" swVersion="Stratum" extends="stratum">
<behaviour api="org.onosproject.net.behaviour.PiPipelineProgrammable"
impl="org.onosproject.drivers.barefoot.TofinoPipelineProgrammable"/>
+ <behaviour api="org.onosproject.net.meter.MeterProgrammable"
+ impl="org.onosproject.drivers.barefoot.TofinoMeterProgrammable"/>
<property name="tableReadCountersWithTableEntries">true</property>
</driver>
diff --git a/drivers/barefoot/src/test/java/org/onosproject/drivers/barefoot/TofinoMeterProgrammableTest.java b/drivers/barefoot/src/test/java/org/onosproject/drivers/barefoot/TofinoMeterProgrammableTest.java
new file mode 100644
index 0000000..b4baf4c
--- /dev/null
+++ b/drivers/barefoot/src/test/java/org/onosproject/drivers/barefoot/TofinoMeterProgrammableTest.java
@@ -0,0 +1,182 @@
+/*
+ * Copyright 2021-present Open Networking Foundation
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.onosproject.drivers.barefoot;
+
+import com.google.common.collect.ImmutableMap;
+import org.junit.Before;
+import org.junit.Test;
+import org.onosproject.net.pi.model.PiMeterId;
+import org.onosproject.net.pi.runtime.PiMeterBand;
+import org.onosproject.net.pi.runtime.PiMeterBandType;
+import org.onosproject.net.pi.runtime.PiMeterCellConfig;
+import org.onosproject.net.pi.runtime.PiMeterCellId;
+
+import java.util.Map;
+
+import static junit.framework.TestCase.assertFalse;
+import static junit.framework.TestCase.assertTrue;
+
+/**
+ * Test for TofinoMeterProgrammable behavior.
+ */
+public class TofinoMeterProgrammableTest {
+
+ private TofinoMeterProgrammable meterProgrammable;
+ private PiMeterCellId meterCellId = PiMeterCellId.ofIndirect(PiMeterId.of("foo"), 0);
+
+ private static final Map<Long, Long> RATES = ImmutableMap.<Long, Long>builder()
+ .put(125000L, 124500L)
+ .put(244800L, 244375L)
+ .put(300000L, 299125L)
+ .build();
+ private static final Map<Long, Long> WRONG_RATES = ImmutableMap.<Long, Long>builder()
+ .put(12500L, 0L)
+ .put(60000L, 0L)
+ .put(124900L, 0L)
+ .put(12501L, 12000L)
+ .put(60001L, 58000L)
+ .put(124901L, 122400L)
+ .put(12502L, 12800L)
+ .put(60002L, 62000L)
+ .put(124902L, 128000L)
+ .build();
+
+ private static final Map<Long, Long> BURSTS = ImmutableMap.<Long, Long>builder()
+ .put(1482L, 1375L)
+ .put(7410L, 7375L)
+ .put(14820L, 14750L)
+ .build();
+
+ private static final Map<Long, Long> WRONG_BURSTS = ImmutableMap.<Long, Long>builder()
+ .put(1482L, 1374L)
+ .put(7410L, 7249L)
+ .put(14820L, 14624L)
+ .put(1483L, 1376L)
+ .put(7411L, 7376L)
+ .put(14821L, 14751L)
+ .build();
+
+ @Before
+ public void setup() {
+ meterProgrammable = new TofinoMeterProgrammable();
+ }
+
+ /**
+ * Test isRateSimilar check of the tofino behavior.
+ */
+ @Test
+ public void testIsRateSimilar() {
+ PiMeterBand onosMeterBand;
+ PiMeterBand deviceMeterBand;
+ PiMeterCellConfig onosMeter;
+ PiMeterCellConfig deviceMeter;
+ for (Map.Entry<Long, Long> entry : RATES.entrySet()) {
+ onosMeterBand = new PiMeterBand(PiMeterBandType.COMMITTED, entry.getKey(), 0);
+ deviceMeterBand = new PiMeterBand(PiMeterBandType.COMMITTED, entry.getValue(), 0);
+ onosMeter = PiMeterCellConfig.builder()
+ .withMeterCellId(meterCellId)
+ .withMeterBand(onosMeterBand)
+ .withMeterBand(new PiMeterBand(PiMeterBandType.PEAK, 0, 0))
+ .build();
+ deviceMeter = PiMeterCellConfig.builder()
+ .withMeterCellId(meterCellId)
+ .withMeterBand(deviceMeterBand)
+ .withMeterBand(new PiMeterBand(PiMeterBandType.PEAK, 0, 0))
+ .build();
+ assertTrue(meterProgrammable.isSimilar(onosMeter, deviceMeter));
+ }
+ }
+
+ /**
+ * Test wrong isRateSimilar of the tofino behavior.
+ */
+ @Test
+ public void testWrongIsRateSimilar() {
+ PiMeterBand onosMeterBand;
+ PiMeterBand deviceMeterBand;
+ PiMeterCellConfig onosMeter;
+ PiMeterCellConfig deviceMeter;
+ for (Map.Entry<Long, Long> entry : WRONG_RATES.entrySet()) {
+ onosMeterBand = new PiMeterBand(PiMeterBandType.COMMITTED, entry.getKey(), 0);
+ deviceMeterBand = new PiMeterBand(PiMeterBandType.COMMITTED, entry.getValue(), 0);
+ onosMeter = PiMeterCellConfig.builder()
+ .withMeterCellId(meterCellId)
+ .withMeterBand(onosMeterBand)
+ .withMeterBand(new PiMeterBand(PiMeterBandType.PEAK, 0, 0))
+ .build();
+ deviceMeter = PiMeterCellConfig.builder()
+ .withMeterCellId(meterCellId)
+ .withMeterBand(deviceMeterBand)
+ .withMeterBand(new PiMeterBand(PiMeterBandType.PEAK, 0, 0))
+ .build();
+ assertFalse(meterProgrammable.isSimilar(onosMeter, deviceMeter));
+ }
+ }
+
+ /**
+ * Test isBurstSimilar of the tofino behavior.
+ */
+ @Test
+ public void testIsBurstSimilar() {
+ PiMeterBand onosMeterBand;
+ PiMeterBand deviceMeterBand;
+ PiMeterCellConfig onosMeter;
+ PiMeterCellConfig deviceMeter;
+ for (Map.Entry<Long, Long> entry : BURSTS.entrySet()) {
+ onosMeterBand = new PiMeterBand(PiMeterBandType.COMMITTED, 0, entry.getKey());
+ deviceMeterBand = new PiMeterBand(PiMeterBandType.COMMITTED, 0, entry.getValue());
+ onosMeter = PiMeterCellConfig.builder()
+ .withMeterCellId(meterCellId)
+ .withMeterBand(onosMeterBand)
+ .withMeterBand(new PiMeterBand(PiMeterBandType.PEAK, 0, 0))
+ .build();
+ deviceMeter = PiMeterCellConfig.builder()
+ .withMeterCellId(meterCellId)
+ .withMeterBand(deviceMeterBand)
+ .withMeterBand(new PiMeterBand(PiMeterBandType.PEAK, 0, 0))
+ .build();
+ assertTrue(meterProgrammable.isSimilar(onosMeter, deviceMeter));
+ }
+ }
+
+ /**
+ * Test wrong isBurstSimilar of the tofino behavior.
+ */
+ @Test
+ public void testWrongIsBurstSimilar() {
+ PiMeterBand onosMeterBand;
+ PiMeterBand deviceMeterBand;
+ PiMeterCellConfig onosMeter;
+ PiMeterCellConfig deviceMeter;
+ for (Map.Entry<Long, Long> entry : WRONG_BURSTS.entrySet()) {
+ onosMeterBand = new PiMeterBand(PiMeterBandType.COMMITTED, 0, entry.getKey());
+ deviceMeterBand = new PiMeterBand(PiMeterBandType.COMMITTED, 0, entry.getValue());
+ onosMeter = PiMeterCellConfig.builder()
+ .withMeterCellId(meterCellId)
+ .withMeterBand(onosMeterBand)
+ .withMeterBand(new PiMeterBand(PiMeterBandType.PEAK, 0, 0))
+ .build();
+ deviceMeter = PiMeterCellConfig.builder()
+ .withMeterCellId(meterCellId)
+ .withMeterBand(deviceMeterBand)
+ .withMeterBand(new PiMeterBand(PiMeterBandType.PEAK, 0, 0))
+ .build();
+ assertFalse(meterProgrammable.isSimilar(onosMeter, deviceMeter));
+ }
+ }
+
+}
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 9a04a35..5165783 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
@@ -227,7 +227,7 @@
}
// The config is not consistent
- if (!translatedEntity.get().translated().equals(config)) {
+ if (!isSimilar(translatedEntity.get().translated(), config)) {
log.warn("Meter Cell Config obtained from device {} is different from " +
"one in in translation store: device={}, store={}",
deviceId, config, translatedEntity.get().translated());
@@ -245,10 +245,10 @@
// 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())
- .forDevice(deviceId)
- .build();
+ .withBands(original.bands())
+ .withCellId(original.meterCellId())
+ .forDevice(deviceId)
+ .build();
meter.setState(MeterState.ADDED);
return meter;
}
@@ -271,6 +271,21 @@
}
/**
+ * Returns true if the given PiMeterCellConfigs are similar enough to be deemed equal
+ * for reconciliation purposes. This is required to handle read/write asymmetry in devices
+ * that allow variations in the meter rate/burst. E.g., devices that implement metering
+ * with a rate or burst size that is slightly higher/lower than the configured ones,
+ * so the values written by ONOS will be different than those read from the device.
+ *
+ * @param onosMeter the ONOS meter
+ * @param deviceMeter the meter in the device
+ * @return true if the meters are similar, false otherwise
+ */
+ protected boolean isSimilar(PiMeterCellConfig onosMeter, PiMeterCellConfig deviceMeter) {
+ return onosMeter.equals(deviceMeter);
+ }
+
+ /**
* P4 meter features builder.
*/
public static class P4RuntimeMeterFeaturesBuilder {