Repository: faucetsdn/faucet
Issue: Meters are only added when FAUCET starts and never removed through config change

When you initialize meters through the yaml file, you see the OFPT_METER_ADD messages sent with the FAUCET event log outputting the following:
```
Sending message with xid(0) to datapath(0000000000000000): version=None,msg_type=None,msg_len=None,xid=0x0,OFPMeterMod(bands=[OFPMeterBandDrop(burst_size=0,len=16,rate=1000,type=1)],command=0,flags=2,meter_id=2)


Sending message with xid(0) to datapath(0000000000000000): version=None,msg_type=None,msg_len=None,xid=0x0,OFPMeterMod(bands=[OFPMeterBandDrop(burst_size=0,len=16,rate=1000,type=1)],command=0,flags=2,meter_id=2)

Sending message with xid(0) to datapath(0000000000000000): version=None,msg_type=None,msg_len=None,xid=0x0,OFPMeterMod(bands=[OFPMeterBandDrop(burst_size=0,len=16,rate=200,type=1)],command=0,flags=1,meter_id=6000)

Sending message with xid(0) to datapath(0000000000000000): version=None,msg_type=None,msg_len=None,xid=0x0,OFPMeterMod(bands=[OFPMeterBandDrop(burst_size=0,len=16,rate=200,type=1)],command=0,flags=1,meter_id=70000)

Sending message with xid(0) to datapath(0000000000000000): version=None,msg_type=None,msg_len=None,xid=0x0,OFPMeterMod(bands=[OFPMeterBandDrop(burst_size=0,len=16,rate=700,type=1)],command=0,flags=1,meter_id=7000)
```


At FAUCET start, I see the OFPT_METER_ADD messages being sent through the control plane where this is expected and correct behavior.

However, if I get rid of the meter through the yaml and send a SIGHUP, I see the following message on event log where the meter I removed is not logged.

```
Sending message with xid(0) to datapath(0000000000000000): version=None,msg_type=None,msg_len=None,xid=0x0,OFPMeterMod(bands=[OFPMeterBandDrop(burst_size=0,len=16,rate=200,type=1)],command=0,flags=1,meter_id=6000)

Sending message with xid(0) to datapath(0000000000000000): version=None,msg_type=None,msg_len=None,xid=0x0,OFPMeterMod(bands=[OFPMeterBandDrop(burst_size=0,len=16,rate=200,type=1)],command=0,flags=1,meter_id=70000)

Sending message with xid(0) to datapath(0000000000000000): version=None,msg_type=None,msg_len=None,xid=0x0,OFPMeterMod(bands=[OFPMeterBandDrop(burst_size=0,len=16,rate=700,type=1)],command=0,flags=1,meter_id=7000)

```

However, I don't see any control plane packets related to metering being sent here. This also means meters are not removed from the switch as well.

I believe the correct behavior we expect to see here is to send a METER_DELETE with the correct METER_ID. Also that meters should be able to be added when there is a new meter added through a config change.

If you stop faucet entirely, restart faucet and reconnect to the switch, a METER DELETE ALL will be sent wiping all of the switch's meters.






Changed files:
faucet/valve_of.py, faucet/valve.py, faucet/dp.py, tests/integration/mininet_tests.py, clib/mininet_test_base.py

diff --git a/clib/mininet_test_base.py b/clib/mininet_test_base.py
index 59a9daaa..3c855ecb 100644
--- a/clib/mininet_test_base.py
+++ b/clib/mininet_test_base.py
@@ -959,6 +959,12 @@ dbs:
             int_dpid, 'stats/portdesc/%s/%s' % (int_dpid, port), timeout)
         return self._port_stat(port_stats, port)
 
+    def get_all_meters_from_dpid(self, dpid):
+        """Return all meters from DPID"""
+        int_dpid = mininet_test_util.str_int_dpid(dpid)
+        return self._ofctl_get(
+            int_dpid, 'stats/meterconfig/%s' % int_dpid, timeout=10)
+
     def wait_matching_in_group_table(self, action, group_id, timeout=10):
         groupdump = os.path.join(self.tmpdir, 'groupdump-%s.txt' % self.dpid)
         for _ in range(timeout):
@@ -973,6 +979,14 @@ dbs:
             time.sleep(1)
         return False
 
+    # TODO: Should this have meter_confs as well or can we just match meter_ids
+    def get_matching_meters_on_dpid(self, dpid):
+        meterdump = os.path.join(self.tmpdir, 'meterdump-%s.log' % dpid)
+        meter_dump = self.get_all_meters_from_dpid(dpid)
+        with open(meterdump, 'w') as meterdump_file:
+            meterdump_file.write(str(meter_dump))
+        return meterdump
+
     def get_matching_flows_on_dpid(self, dpid, match, table_id, timeout=10,
                                    actions=None, hard_timeout=0, cookie=None,
                                    ofa_match=True):
@@ -2509,6 +2523,17 @@ dbs:
             time.sleep(1)
         self.fail('%s not found in %s (%d/%d)' % (exp, log_name, len(lines), count))
 
+    def wait_until_no_matching_lines_from_file(self, exp, log_name, timeout=30, count=1):
+        """Require (count) matching lines to be non-existent in file."""
+        assert timeout >= 1
+        for _ in range(timeout):
+            if os.path.exists(log_name):
+                lines = self.matching_lines_from_file(exp, log_name)
+                if len(lines) >= count:
+                    return self.fail('%s found in %s (%d/%d)' % (exp, log_name, len(lines), count))
+            time.sleep(1)
+        return lines
+
     def exabgp_updates(self, exabgp_log, timeout=60):
         """Verify that exabgp process has received BGP updates."""
         controller = self._get_controller()
diff --git a/faucet/dp.py b/faucet/dp.py
index dc19be7b..448ae383 100644
--- a/faucet/dp.py
+++ b/faucet/dp.py
@@ -1482,10 +1482,24 @@ configuration.
             if changed_acl_ports:
                 same_ports -= changed_acl_ports
                 logger.info('ports where ACL only changed: %s' % changed_acl_ports)
-
         return (all_ports_changed, deleted_ports,
                 added_ports.union(changed_ports), changed_acl_ports)
 
+    def _get_meter_config_changes(self, logger, new_dp):
+        """Detect any config changes to meters.
+        Args:
+            logger (ValveLogger): logger instance.
+            new_dp (DP): new dataplane configuration.
+        Returns:
+            changes (tuple) of:
+                deleted_meters (set): deleted Meter IDs.
+                changed_meters (set): changed/added Meter IDs.
+        """
+        all_meters_changed, deleted_meters, added_meters, changed_meters, _ = self._get_conf_changes(
+            logger, 'METERS', self.meters, new_dp.meters)
+
+        return (all_meters_changed, deleted_meters, added_meters, changed_meters)
+
     def get_config_changes(self, logger, new_dp):
         """Detect any config changes.
 
@@ -1501,6 +1515,8 @@ configuration.
                 deleted_vlans (set): deleted VLAN IDs.
                 changed_vlans (set): changed/added VLAN IDs.
                 all_ports_changed (bool): True if all ports changed.
+                deleted_meters (set): deleted meter numbers
+                changed_meters (set): changed/added meter numbers
         """
         def _table_configs(dp):
             return frozenset([
@@ -1517,13 +1533,17 @@ configuration.
         else:
             changed_acls = self._get_acl_config_changes(logger, new_dp)
             deleted_vlans, changed_vlans = self._get_vlan_config_changes(logger, new_dp)
-            (all_ports_changed, deleted_ports,
-             changed_ports, changed_acl_ports) = self._get_port_config_changes(
+            (all_ports_changed, deleted_ports, changed_ports,
+             changed_acl_ports) = self._get_port_config_changes(
                  logger, new_dp, changed_vlans, changed_acls)
+            (all_meters_changed, deleted_meters,
+             added_meters, changed_meters) = self._get_meter_config_changes(logger, new_dp)
             return (deleted_ports, changed_ports, changed_acl_ports,
-                    deleted_vlans, changed_vlans, all_ports_changed)
+                    deleted_vlans, changed_vlans, all_ports_changed,
+                    all_meters_changed, deleted_meters,
+                    added_meters, changed_meters)
         # default cold start
-        return (set(), set(), set(), set(), set(), True)
+        return (set(), set(), set(), set(), set(), True, True, set(), set(), set())
 
     def get_tables(self):
         """Return tables as dict for API call."""
diff --git a/faucet/valve.py b/faucet/valve.py
index f0e37f11..aeeea53b 100644
--- a/faucet/valve.py
+++ b/faucet/valve.py
@@ -1643,13 +1643,19 @@ class Valve:
                 deleted_vids (set): deleted VLAN IDs.
                 changed_vids (set): changed/added VLAN IDs.
                 all_ports_changed (bool): True if all ports changed.
+                all_meters_changed (bool): True if all meters changed.
+                deleted_meters: (set): deleted meter numbers.
+                changed_meters: (set): changed meter numbers.
+                added_meters: (set): added meter numbers.
         Returns:
             tuple:
                 cold_start (bool): whether cold starting.
                 ofmsgs (list): OpenFlow messages.
         """
         (deleted_ports, changed_ports, changed_acl_ports,
-         deleted_vids, changed_vids, all_ports_changed) = changes
+         deleted_vids, changed_vids, all_ports_changed,
+         all_meters_changed, deleted_meters, added_meters,
+         changed_meters) = changes
 
         if self._pipeline_change():
             self.logger.info('pipeline change')
@@ -1667,13 +1673,38 @@ class Valve:
 
         ofmsgs = []
 
+        # TODO: optimize for all meters being erased
         if deleted_ports:
             ofmsgs.extend(self.ports_delete(deleted_ports))
         if deleted_vids:
             deleted_vlans = [self.dp.vlans[vid] for vid in deleted_vids]
             ofmsgs.extend(self.del_vlans(deleted_vlans))
+        if deleted_meters:
+            for meter_key in self.dp.meters.keys():
+                if meter_key in deleted_meters:
+                    ofmsgs.append(valve_of.meterdel(
+                        meter_id=self.dp.meters.get(meter_key).meter_id))
+
         if changed_ports:
             ofmsgs.extend(self.ports_delete(changed_ports))
+        if changed_meters:
+            for changed_meter in changed_meters:
+                for dp_meter_key in self.dp.meters.keys():
+                    if (changed_meter == dp_meter_key) and (
+                        new_dp.meters.get(changed_meter).meter_id != self.dp.meters.get(
+                            changed_meter).meter_id):
+                        ofmsgs.append(valve_of.meterdel(
+                            meter_id=self.dp.meters.get(changed_meter).meter_id))
+                        ofmsgs.append(valve_of.meteradd(
+                                new_dp.meters.get(changed_meter).entry, command=0))
+                    else:
+                        ofmsgs.append(valve_of.meteradd(
+                            new_dp.meters.get(changed_meter).entry, command=1))
+
+        if added_meters:
+            for added_meter in added_meters:
+                ofmsgs.append(valve_of.meteradd(
+                    new_dp.meters.get(added_meter).entry, command=0))
 
         self.dp_init(new_dp)
 
diff --git a/faucet/valve_of.py b/faucet/valve_of.py
index d7fae3c9..d70a5a1b 100644
--- a/faucet/valve_of.py
+++ b/faucet/valve_of.py
@@ -831,7 +831,7 @@ def meterdel(datapath=None, meter_id=ofp.OFPM_ALL):
         meter_id)
 
 
-def meteradd(meter_conf):
+def meteradd(meter_conf, command=ofp.OFPMC_ADD):
     """Add a meter based on YAML configuration."""
 
     class NoopDP:
@@ -852,12 +852,11 @@ def meteradd(meter_conf):
             msg.xid = 0
 
     noop_dp = NoopDP()
-    ofctl.mod_meter_entry(noop_dp, meter_conf, ofp.OFPMC_ADD)
+    ofctl.mod_meter_entry(noop_dp, meter_conf, command)
     noop_dp.msg.xid = None
     noop_dp.msg.datapath = None
     return noop_dp.msg
 
-
 def controller_pps_meteradd(datapath=None, pps=0):
     """Add a PPS meter towards controller."""
     return parser.OFPMeterMod(
diff --git a/tests/integration/mininet_tests.py b/tests/integration/mininet_tests.py
index 194746f6..16abad5c 100644
--- a/tests/integration/mininet_tests.py
+++ b/tests/integration/mininet_tests.py
@@ -1685,6 +1685,62 @@ class FaucetUntaggedApplyMeterTest(FaucetUntaggedMeterParseTest):
         self.assertTrue(byte_band_count)
 
 
+class FaucetUntaggedMeterAddTest(FaucetUntaggedMeterParseTest):
+
+    def test_untagged(self):
+        super(FaucetUntaggedMeterAddTest, self).test_untagged()
+        conf = self._get_faucet_conf()
+        del conf['acls']
+        conf['meters']['lossymeter2'] = {
+            'meter_id': 2,
+            'entry': {
+                'flags': ['PKTPS'],
+                'bands': [{'rate': '1000', 'type': 'DROP'}]
+            },
+        }
+        self.reload_conf(
+            conf, self.faucet_config_path,
+            restart=True, cold_start=False, change_expected=True, hup=True)
+        self.wait_until_matching_lines_from_file(
+            r'.+\\'meter_id\\'\\: 2+',
+            self.get_matching_meters_on_dpid(self.dpid))
+
+
+class FaucetUntaggedMeterDeleteTest(FaucetUntaggedMeterParseTest):
+
+    def test_untagged(self):
+        super(FaucetUntaggedMeterDeleteTest, self).test_untagged()
+        conf = self._get_faucet_conf()
+        del conf['meters']['lossymeter']
+        self.reload_conf(
+            conf, self.faucet_config_path,
+            restart=True, cold_start=False, change_expected=True)
+        self.wait_until_no_matching_lines_from_file(
+            r'.+meter_id+',
+            self.get_matching_meters_on_dpid(self.dpid))
+
+
+class FaucetUntaggedMeterModTest(FaucetUntaggedMeterParseTest):
+
+    def test_untagged(self):
+        super(FaucetUntaggedMeterModTest, self).test_untagged()
+        conf = self._get_faucet_conf()
+        del conf['acls']
+        conf['meters']['lossymeter'] = {
+            'meter_id': 1,
+            'entry': {
+                'flags': ['PKTPS'],
+                'bands': [{'rate': '1000', 'type': 'DROP'}]
+            },
+        }
+        self.reload_conf(
+            conf, self.faucet_config_path,
+            restart=True, cold_start=False, change_expected=True, hup=True)
+        self.wait_until_matching_lines_from_file(
+            r'.+PKTPS+',
+            self.get_matching_meters_on_dpid(self.dpid))
+
+
 class FaucetUntaggedHairpinTest(FaucetUntaggedTest):
 
     NETNS = True