Repository: telstra/open-kilda
Issue: Kilda wouldn't reinstall meter with incorrect burst size

**Steps to reproduce (for local env):**
1. Deploy Kilda with any topology
2. Connect to lab_service container `docker exec -it lab_service-1 /bin/bash`
3. Remove broadcast rule `ovs-ofctl -O OpenFlow13 --strict del-flows ofsw2 priority=31768,udp,dl_dst=00:26:e1:ff:ff:ff,tp_dst=61231`
4. Modify broadcast meter `ovs-ofctl -O OpenFlow13 mod-meter ofsw2 meter=2,pktps,burst,stats,bands=type=drop,rate=200,burst_size=5000`
5. Validate switch. You will get 1 missing rule and 1 misconfigured meter in response. (expected bust size 4096, actual - 5000)
6. Synchronize switch
7. Validate switch

**Expected result:**
0 missing rules and 0 misconfigured rules

**Actual result:**
0 missing rules, 1 misconfigured rules with incorrect burst size.

**Reson:**
During rule installation Kilda checks existing meters. If meter with needed ID exists Kilda checks meter parameters: rate and flags (but **not** burst size). If parameters are correct - Kilda just skips meter step. If parameters are different - Kilda removes old meter and installs new one. To fix the problem Kilda must check burst size too. 

User can face with this problem during switch flapping. Some times after flapping we got incorrect burst sizes for system meters and Kilda just skips fixing of such meters during switch init synchronization. 

Changed files:
src-java/floodlight-service/floodlight-modules/src/main/java/org/openkilda/floodlight/switchmanager/SwitchManager.java
src-java/floodlight-service/floodlight-modules/src/test/java/org/openkilda/floodlight/switchmanager/SwitchManagerTest.java

diff --git a/src-java/floodlight-service/floodlight-modules/src/main/java/org/openkilda/floodlight/switchmanager/SwitchManager.java b/src-java/floodlight-service/floodlight-modules/src/main/java/org/openkilda/floodlight/switchmanager/SwitchManager.java
index 28ee1be5d..49406e77d 100644
--- a/src-java/floodlight-service/floodlight-modules/src/main/java/org/openkilda/floodlight/switchmanager/SwitchManager.java
+++ b/src-java/floodlight-service/floodlight-modules/src/main/java/org/openkilda/floodlight/switchmanager/SwitchManager.java
@@ -2267,35 +2267,39 @@ public class SwitchManager implements IFloodlightModule, IFloodlightService, ISw
     @VisibleForTesting
     void processMeter(IOFSwitch sw, OFMeterMod meterMod) {
         long meterId = meterMod.getMeterId();
-        OFMeterConfig meterConfig;
+        OFMeterConfig actualMeterConfig;
         try {
-            meterConfig = getMeter(sw.getId(), meterId);
+            actualMeterConfig = getMeter(sw.getId(), meterId);
         } catch (SwitchOperationException e) {
             logger.warn("Meter {} won't be installed on the switch {}: {}", meterId, sw.getId(), e.getMessage());
             return;
         }
 
-        OFMeterBandDrop meterBandDrop = Optional.ofNullable(meterConfig)
+        OFMeterBandDrop actualMeterBandDrop = Optional.ofNullable(actualMeterConfig)
                 .map(OFMeterConfig::getEntries)
                 .flatMap(entries -> entries.stream().findFirst())
                 .map(OFMeterBandDrop.class::cast)
                 .orElse(null);
 
         try {
-            OFMeterBandDrop ofMeterBandDrop = sw.getOFFactory().getVersion().compareTo(OF_13) > 0
+            OFMeterBandDrop expectedMeterBandDrop = sw.getOFFactory().getVersion().compareTo(OF_13) > 0
                     ? (OFMeterBandDrop) meterMod.getBands().get(0) : (OFMeterBandDrop) meterMod.getMeters().get(0);
-            long rate = ofMeterBandDrop.getRate();
-            Set<OFMeterFlags> flags = meterMod.getFlags();
+            long expectedRate = expectedMeterBandDrop.getRate();
+            long expectedBurstSize = expectedMeterBandDrop.getBurstSize();
+            Set<OFMeterFlags> expectedFlags = meterMod.getFlags();
 
-            if (meterBandDrop != null && meterBandDrop.getRate() == rate
-                    && CollectionUtils.isEqualCollection(meterConfig.getFlags(), flags)) {
+            if (actualMeterBandDrop != null && actualMeterBandDrop.getRate() == expectedRate
+                    && actualMeterBandDrop.getBurstSize() == expectedBurstSize
+                    && CollectionUtils.isEqualCollection(actualMeterConfig.getFlags(), expectedFlags)) {
                 logger.debug("Meter {} won't be reinstalled on switch {}. It already exists", meterId, sw.getId());
                 return;
             }
 
-            if (meterBandDrop != null) {
-                logger.info("Meter {} with origin rate {} will be reinstalled on {} switch.",
-                        meterId, sw.getId(), meterBandDrop.getRate());
+            if (actualMeterBandDrop != null) {
+                logger.info("Meter {} on switch {} has rate={}, burst size={} and flags={} but it must have "
+                                + "rate={}, burst size={} and flags={}. Meter will be reinstalled.",
+                        meterId, sw.getId(), actualMeterBandDrop.getRate(), actualMeterBandDrop.getBurstSize(),
+                        actualMeterConfig.getFlags(), expectedRate, expectedBurstSize, expectedFlags);
                 buildAndDeleteMeter(sw, sw.getId(), meterId);
                 sendBarrierRequest(sw);
             }
diff --git a/src-java/floodlight-service/floodlight-modules/src/test/java/org/openkilda/floodlight/switchmanager/SwitchManagerTest.java b/src-java/floodlight-service/floodlight-modules/src/test/java/org/openkilda/floodlight/switchmanager/SwitchManagerTest.java
index 5f612baa3..70d985e48 100644
--- a/src-java/floodlight-service/floodlight-modules/src/test/java/org/openkilda/floodlight/switchmanager/SwitchManagerTest.java
+++ b/src-java/floodlight-service/floodlight-modules/src/test/java/org/openkilda/floodlight/switchmanager/SwitchManagerTest.java
@@ -282,7 +282,7 @@ public class SwitchManagerTest {
 
     public void runInstallVerificationBroadcastRule(boolean supportsUdpPortMatch) throws Exception {
         mockGetGroupsRequest(ImmutableList.of(GroupId.ROUND_TRIP_LATENCY_GROUP_ID.intValue()));
-        mockGetMetersRequest(ImmutableList.of(meterId), true, 10L);
+        mockGetMetersRequest(ImmutableList.of(meterId), true, 10L, 100);
         mockFlowStatsRequest(VERIFICATION_BROADCAST_RULE_COOKIE);
         mockBarrierRequest();
 
@@ -316,7 +316,7 @@ public class SwitchManagerTest {
 
     @Test
     public void installVerificationUnicastRule() throws Exception {
-        mockGetMetersRequest(Lists.newArrayList(broadcastMeterId), true, 10L);
+        mockGetMetersRequest(Lists.newArrayList(broadcastMeterId), true, 10L, 100);
         mockBarrierRequest();
         expect(iofSwitch.write(anyObject(OFMeterMod.class))).andReturn(true).times(1);
         Capture<OFFlowMod> capture = prepareForInstallTest();
@@ -423,7 +423,7 @@ public class SwitchManagerTest {
 
     @Test
     public void installUnicastVerificationRuleVxlan() throws Exception {
-        mockGetMetersRequest(Lists.newArrayList(broadcastMeterId), true, 10L);
+        mockGetMetersRequest(Lists.newArrayList(broadcastMeterId), true, 10L, 100);
         mockBarrierRequest();
         expect(iofSwitch.write(anyObject(OFMeterMod.class))).andReturn(true).times(1);
         Capture<OFFlowMod> capture = prepareForInstallTest();
@@ -661,7 +661,6 @@ public class SwitchManagerTest {
 
         mockBarrierRequest();
         mockFlowStatsRequest(cookie);
-        mockGetMetersRequest(Collections.emptyList(), true, 0);
 
         replay(ofSwitchService, iofSwitch, switchDescription);
 
@@ -1064,7 +1063,7 @@ public class SwitchManagerTest {
         // define that switch is Centec
         expect(switchDescription.getManufacturerDescription()).andStubReturn("Centec Inc.");
         expect(featureDetectorService.detectSwitch(iofSwitch)).andStubReturn(Sets.newHashSet(METERS));
-        mockGetMetersRequest(Collections.emptyList(), true, 0);
+        mockGetMetersRequest(Collections.emptyList(), true, 0, 0);
         mockBarrierRequest();
 
         Capture<OFMeterMod> capture = EasyMock.newCapture(CaptureType.ALL);
@@ -1105,7 +1104,8 @@ public class SwitchManagerTest {
         expect(switchDescription.getManufacturerDescription()).andStubReturn("Centec Inc.");
         expect(featureDetectorService.detectSwitch(iofSwitch)).andStubReturn(Sets.newHashSet(METERS));
         mockBarrierRequest();
-        mockGetMetersRequest(Lists.newArrayList(unicastMeterId), false, expectedRate);
+        mockGetMetersRequest(Lists.newArrayList(unicastMeterId), false, expectedRate,
+                config.getSystemMeterBurstSizeInPackets());
 
         Capture<OFMeterMod> capture = EasyMock.newCapture(CaptureType.ALL);
         expect(iofSwitch.write(capture(capture))).andReturn(true).times(2);
@@ -1113,7 +1113,7 @@ public class SwitchManagerTest {
         replay(ofSwitchService, iofSwitch, switchDescription, featureDetectorService);
 
         // when
-        Set<OFMeterFlags> flags = ImmutableSet.of(OFMeterFlags.KBPS, OFMeterFlags.STATS, OFMeterFlags.BURST);
+        Set<OFMeterFlags> flags = ImmutableSet.of(OFMeterFlags.KBPS, OFMeterFlags.BURST);
         OFMeterMod ofMeterMod = buildMeterMod(iofSwitch.getOFFactory(), expectedRate,
                 config.getSystemMeterBurstSizeInPackets(), unicastMeterId, flags);
         switchManager.processMeter(iofSwitch, ofMeterMod);
@@ -1148,7 +1148,8 @@ public class SwitchManagerTest {
         expect(iofSwitch.write(capture(capture))).andReturn(true).times(2);
 
         mockBarrierRequest();
-        mockGetMetersRequest(Lists.newArrayList(unicastMeter), true, originRate);
+        mockGetMetersRequest(Lists.newArrayList(unicastMeter), true, originRate,
+                config.getSystemMeterBurstSizeInPackets());
         replay(ofSwitchService, iofSwitch, switchDescription, featureDetectorService);
 
         //when
@@ -1168,6 +1169,45 @@ public class SwitchManagerTest {
         assertThat(actual.get(1), hasProperty("flags", containsInAnyOrder(flags.toArray())));
     }
 
+    @Test
+    public void shouldRenstallMetersIfBurstSizeIsUpdated() throws Exception {
+        long unicastMeter = createMeterIdForDefaultRule(VERIFICATION_UNICAST_RULE_COOKIE).getValue();
+        long originBurstSize = config.getSystemMeterBurstSizeInPackets();
+        long updatedBurstSize = config.getSystemMeterBurstSizeInPackets() + 10;
+
+        // given
+        expect(ofSwitchService.getActiveSwitch(dpid)).andStubReturn(iofSwitch);
+        expect(iofSwitch.getOFFactory()).andStubReturn(ofFactory);
+        expect(iofSwitch.getSwitchDescription()).andStubReturn(switchDescription);
+        expect(iofSwitch.getId()).andStubReturn(dpid);
+        expect(switchDescription.getManufacturerDescription()).andStubReturn(StringUtils.EMPTY);
+        expect(featureDetectorService.detectSwitch(iofSwitch)).andStubReturn(Sets.newHashSet(PKTPS_FLAG));
+        Capture<OFMeterMod> capture = EasyMock.newCapture(CaptureType.ALL);
+        // 1 meter deletion + 1 meters installation
+        expect(iofSwitch.write(capture(capture))).andReturn(true).times(2);
+
+        mockBarrierRequest();
+        mockGetMetersRequest(Lists.newArrayList(unicastMeter), true, config.getUnicastRateLimit(),
+                originBurstSize);
+        replay(ofSwitchService, iofSwitch, switchDescription, featureDetectorService);
+
+        //when
+        Set<OFMeterFlags> flags = ImmutableSet.of(OFMeterFlags.PKTPS, OFMeterFlags.STATS, OFMeterFlags.BURST);
+        OFMeterMod ofMeterMod = buildMeterMod(iofSwitch.getOFFactory(), config.getUnicastRateLimit(),
+                updatedBurstSize, unicastMeter, flags);
+        switchManager.processMeter(iofSwitch, ofMeterMod);
+
+        final List<OFMeterMod> actual = capture.getValues();
+        assertEquals(2, actual.size());
+
+        // verify meters deletion
+        assertThat(actual.get(0), hasProperty("command", equalTo(OFMeterModCommand.DELETE)));
+        // verify meter installation
+        assertThat(actual.get(1), hasProperty("command", equalTo(OFMeterModCommand.ADD)));
+        assertThat(actual.get(1), hasProperty("meterId", equalTo(unicastMeter)));
+        assertThat(actual.get(1), hasProperty("flags", containsInAnyOrder(flags.toArray())));
+    }
+
     @Test
     public void shouldNotInstallMetersIfAlreadyExists() throws Exception {
         long expectedRate = config.getBroadcastRateLimit();
@@ -1185,7 +1225,8 @@ public class SwitchManagerTest {
                 .andReturn(Sets.newHashSet(GROUP_PACKET_OUT_CONTROLLER, NOVIFLOW_COPY_FIELD, PKTPS_FLAG))
                 .times(8);
         mockBarrierRequest();
-        mockGetMetersRequest(Lists.newArrayList(unicastMeterId, broadcastMeterId), true, expectedRate);
+        mockGetMetersRequest(Lists.newArrayList(unicastMeterId, broadcastMeterId), true, expectedRate,
+                config.getBroadcastRateLimit());
         mockGetGroupsRequest(Lists.newArrayList(GroupId.ROUND_TRIP_LATENCY_GROUP_ID.intValue()));
         replay(ofSwitchService, iofSwitch, switchDescription, featureDetectorService);
 
@@ -1309,17 +1350,19 @@ public class SwitchManagerTest {
         return capture;
     }
 
-    private void mockGetMetersRequest(List<Long> meterIds, boolean supportsPkts, long rate) throws Exception {
+    private void mockGetMetersRequest(List<Long> meterIds, boolean supportsPkts, long rate, long burstSize)
+            throws Exception {
         List<OFMeterConfig> meterConfigs = new ArrayList<>(meterIds.size());
         for (Long meterId : meterIds) {
             OFMeterBandDrop bandDrop = mock(OFMeterBandDrop.class);
             expect(bandDrop.getRate()).andStubReturn(rate);
+            expect(bandDrop.getBurstSize()).andStubReturn(burstSize);
 
             OFMeterConfig meterConfig = mock(OFMeterConfig.class);
             expect(meterConfig.getEntries()).andStubReturn(Collections.singletonList(bandDrop));
             expect(meterConfig.getMeterId()).andStubReturn(meterId);
 
-            Set<OFMeterFlags> flags = ImmutableSet.of(OFMeterFlags.STATS,
+            Set<OFMeterFlags> flags = ImmutableSet.of(OFMeterFlags.STATS, OFMeterFlags.BURST,
                     supportsPkts ? OFMeterFlags.PKTPS : OFMeterFlags.KBPS);
             expect(meterConfig.getFlags()).andStubReturn(flags);
             replay(bandDrop, meterConfig);