[OVN] Update ovn meter when neutron server reloads

Up until now, we needed to remove all logging objects to see the
meter-band properties being changed after a server restart. Now we check
for inconsistencies between the neutron configuration and the OVN
meter-band object after a restart. The function create_ovn_fair_meter is
now located in the ovn_driver instead of the log_driver so as to be able
to call it from the maintenance task.

Conflicts:
    neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/maintenance.py
    neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_client.py
    neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_ovn_client.py

Closes-bug: #2017145
Signed-off-by: Elvira García <egarciar@redhat.com>
Change-Id: I24cef85ed68c893a740445707f88296d763c8de8
(cherry picked from commit c3602ac)

neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/maintenance.py

@@ -41,6 +41,7 @@
 from neutron.objects import ports as ports_obj
 from neutron.objects import router as router_obj
 from neutron.plugins.ml2.drivers.ovn.mech_driver.ovsdb import ovn_db_sync
+from neutron.services.logapi.drivers.ovn import driver as log_driver
 
 
 CONF = cfg.CONF
@@ -956,6 +957,24 @@ def create_router_extra_attributes_registers(self):
 
         raise periodics.NeverAgain()
 
+    @periodics.periodic(spacing=600, run_immediately=True)
+    def check_fair_meter_consistency(self):
+        """Update the logging meter after neutron-server reload
+
+        When we change the rate and burst limit we need to update the fair
+        meter band to apply the new values. This is called from the ML2/OVN
+        driver after the OVN NB idl is loaded
+
+        """
+        if not self.has_lock:
+            return
+        if log_driver.OVNDriver.network_logging_supported(self._nb_idl):
+            meter_name = (
+                cfg.CONF.network_log.local_output_log_base or "acl_log_meter")
+            self._ovn_client.create_ovn_fair_meter(meter_name,
+                                                   from_reload=True)
+        raise periodics.NeverAgain()
+
 
 class HashRingHealthCheckPeriodics(object):
 

neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_client.py

@@ -31,6 +31,7 @@
 from neutron_lib.plugins import constants as plugin_constants
 from neutron_lib.plugins import directory
 from neutron_lib.plugins import utils as p_utils
+from neutron_lib.services.logapi import constants as log_const
 from neutron_lib.utils import helpers
 from neutron_lib.utils import net as n_net
 from oslo_config import cfg
@@ -2650,3 +2651,54 @@ def add_txns_to_remove_port_dns_records(self, txn, port):
             if ls_dns_record.records.get(ptr_record):
                 txn.add(self._nb_idl.dns_remove_record(
                         ls_dns_record.uuid, ptr_record, if_exists=True))
+
+    def create_ovn_fair_meter(self, meter_name, from_reload=False, txn=None):
+        """Create row in Meter table with fair attribute set to True.
+
+        Create a row in OVN's NB Meter table based on well-known name. This
+        method uses the network_log configuration to specify the attributes
+        of the meter. Current implementation needs only one 'fair' meter row
+        which is then referred by multiple ACL rows.
+
+        :param meter_name: ovn northbound meter name.
+        :param from_reload: whether we update the meter values or create them.
+        :txn: ovn northbound idl transaction.
+
+        """
+        meter = self._nb_idl.db_find_rows(
+            "Meter", ("name", "=", meter_name)).execute(check_error=True)
+        # The meter is created when a log object is created, not by default.
+        # This condition avoids creating the meter if it wasn't there already
+        commands = []
+        if from_reload and not meter:
+            return
+        if meter:
+            meter = meter[0]
+            meter_band = self._nb_idl.lookup("Meter_Band",
+                                             meter.bands[0].uuid, default=None)
+            if meter_band:
+                if all((meter.unit == "pktps",
+                        meter.fair[0],
+                        meter_band.rate == cfg.CONF.network_log.rate_limit,
+                        meter_band.burst_size ==
+                        cfg.CONF.network_log.burst_limit)):
+                    # Meter (and its meter-band) unchanged: noop.
+                    return
+            # Re-create meter (and its meter-band) with the new attributes.
+            # This is supposed to happen only if configuration changed, so
+            # doing updates is an overkill: better to leverage the ovsdbapp
+            # library to avoid the complexity.
+            LOG.info("Deleting outdated log fair meter %s", meter_name)
+            commands.append(self._nb_idl.meter_del(meter.uuid))
+        # Create meter
+        LOG.info("Creating network log fair meter %s", meter_name)
+        commands.append(self._nb_idl.meter_add(
+                        name=meter_name,
+                        unit="pktps",
+                        rate=cfg.CONF.network_log.rate_limit,
+                        fair=True,
+                        burst_size=cfg.CONF.network_log.burst_limit,
+                        may_exist=False,
+                        external_ids={ovn_const.OVN_DEVICE_OWNER_EXT_ID_KEY:
+                                      log_const.LOGGING_PLUGIN}))
+        self._transaction(commands, txn=txn)

neutron/services/logapi/drivers/ovn/driver.py

@@ -89,54 +89,14 @@ def _get_logs(self, context):
         log_objs = self._log_plugin.get_logs(context)
         return [self._log_dict_to_obj(lo) for lo in log_objs]
 
+    @property
+    def _ovn_client(self):
+        return self.plugin_driver._ovn_client
+
     @property
     def ovn_nb(self):
         return self.plugin_driver.nb_ovn
 
-    def _create_ovn_fair_meter(self, ovn_txn):
-        """Create row in Meter table with fair attribute set to True.
-
-        Create a row in OVN's NB Meter table based on well-known name. This
-        method uses the network_log configuration to specify the attributes
-        of the meter. Current implementation needs only one 'fair' meter row
-        which is then referred by multiple ACL rows.
-
-        :param ovn_txn: ovn northbound idl transaction.
-
-        """
-        meter = self.ovn_nb.db_find_rows(
-            "Meter", ("name", "=", self.meter_name)).execute(check_error=True)
-        if meter:
-            meter = meter[0]
-            try:
-                meter_band = self.ovn_nb.lookup("Meter_Band",
-                                                meter.bands[0].uuid)
-                if all((meter.unit == "pktps",
-                        meter.fair[0],
-                        meter_band.rate == cfg.CONF.network_log.rate_limit,
-                        meter_band.burst_size ==
-                        cfg.CONF.network_log.burst_limit)):
-                    # Meter (and its meter-band) unchanged: noop.
-                    return
-            except idlutils.RowNotFound:
-                pass
-            # Re-create meter (and its meter-band) with the new attributes.
-            # This is supposed to happen only if configuration changed, so
-            # doing updates is an overkill: better to leverage the ovsdbapp
-            # library to avoid the complexity.
-            ovn_txn.add(self.ovn_nb.meter_del(meter.uuid))
-        # Create meter
-        LOG.info("Creating network log fair meter %s", self.meter_name)
-        ovn_txn.add(self.ovn_nb.meter_add(
-            name=self.meter_name,
-            unit="pktps",
-            rate=cfg.CONF.network_log.rate_limit,
-            fair=True,
-            burst_size=cfg.CONF.network_log.burst_limit,
-            may_exist=False,
-            external_ids={ovn_const.OVN_DEVICE_OWNER_EXT_ID_KEY:
-                          log_const.LOGGING_PLUGIN}))
-
     @staticmethod
     def _acl_actions_enabled(log_obj):
         if not log_obj.enabled:
@@ -303,7 +263,8 @@ def create_log(self, context, log_obj):
         pgs = self._pgs_from_log_obj(context, log_obj)
         actions_enabled = self._acl_actions_enabled(log_obj)
         with self.ovn_nb.transaction(check_error=True) as ovn_txn:
-            self._create_ovn_fair_meter(ovn_txn)
+            self._ovn_client.create_ovn_fair_meter(self.meter_name,
+                                                   txn=ovn_txn)
             self._set_acls_log(pgs, ovn_txn, actions_enabled,
                                utils.ovn_name(log_obj.id))
 

neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_maintenance.py

@@ -30,9 +30,15 @@
 from neutron.db import ovn_revision_numbers_db as db_rev
 from neutron.plugins.ml2.drivers.ovn.mech_driver.ovsdb import maintenance
 from neutron.tests.functional import base
+from neutron.tests.functional.services.logapi.drivers.ovn \
+    import test_driver as test_log_driver
+
 from neutron.tests.unit.api import test_extensions
 from neutron.tests.unit.extensions import test_extraroute
 
+CFG_NEW_BURST = 50
+CFG_NEW_RATE = 150
+
 
 class _TestMaintenanceHelper(base.TestOVNFunctionalBase):
     """A helper class to keep the code more organized."""
@@ -943,3 +949,37 @@ def _verify_lb(test, protocol, vip_ext_port, vip_int_port):
 
             # Assert load balancer for port forwarding is gone
             self.assertFalse(self._find_pf_lb(router_id, fip_id))
+
+
+class TestLogMaintenance(_TestMaintenanceHelper,
+                         test_log_driver.LogApiTestCaseBase):
+    def test_check_for_logging_conf_change(self):
+        # Check logging is supported
+        if not self.log_driver.network_logging_supported(self.nb_api):
+            self.skipTest("The current OVN version does not offer support "
+                          "for neutron network log functionality.")
+            self.assertIsNotNone(self.log_plugin)
+        # Check no meter exists
+        self.assertFalse(self.nb_api._tables['Meter'].rows.values())
+        # Add a log object
+        self.log_plugin.create_log(self.context, self._log_data())
+        # Check a meter and fair meter exist
+        self.assertTrue(self.nb_api._tables['Meter'].rows)
+        self.assertTrue(self.nb_api._tables['Meter_Band'].rows)
+        self.assertEqual(cfg.CONF.network_log.burst_limit,
+            [*self.nb_api._tables['Meter_Band'].rows.values()][0].burst_size)
+        self.assertEqual(cfg.CONF.network_log.rate_limit,
+            [*self.nb_api._tables['Meter_Band'].rows.values()][0].rate)
+        # Update burst and rate limit values on the configuration
+        ovn_config.cfg.CONF.set_override('burst_limit', CFG_NEW_BURST,
+                                         group='network_log')
+        ovn_config.cfg.CONF.set_override('rate_limit', CFG_NEW_RATE,
+                                         group='network_log')
+        # Call the maintenance task
+        self.assertRaises(periodics.NeverAgain,
+                          self.maint.check_fair_meter_consistency)
+        # Check meter band was effectively changed after the maintenance call
+        self.assertEqual(CFG_NEW_BURST,
+            [*self.nb_api._tables['Meter_Band'].rows.values()][0].burst_size)
+        self.assertEqual(CFG_NEW_RATE,
+            [*self.nb_api._tables['Meter_Band'].rows.values()][0].rate)

neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_ovn_client.py

@@ -20,7 +20,10 @@
 from neutron.plugins.ml2.drivers.ovn.mech_driver.ovsdb import ovn_client
 from neutron.tests import base
 from neutron.tests.unit import fake_resources as fakes
+from neutron.tests.unit.services.logapi.drivers.ovn \
+    import test_driver as test_log_driver
 from neutron_lib.api.definitions import portbindings
+from neutron_lib.services.logapi import constants as log_const
 
 
 class TestOVNClientBase(base.BaseTestCase):
@@ -122,3 +125,61 @@ def test_vnic_remote_managed_port_context(self):
         self.assertEqual(
             self.fake_smartnic_hostname,
             self.ovn_client.determine_bind_host({}, port_context=context))
+
+
+class TestOVNClientFairMeter(TestOVNClientBase,
+                             test_log_driver.TestOVNDriverBase):
+
+    def test_create_ovn_fair_meter(self):
+        mock_find_rows = mock.Mock()
+        mock_find_rows.execute.return_value = None
+        self.nb_idl.db_find_rows.return_value = mock_find_rows
+        self.ovn_client.create_ovn_fair_meter(self._log_driver.meter_name)
+        self.assertFalse(self.nb_idl.meter_del.called)
+        self.assertTrue(self.nb_idl.meter_add.called)
+        self.nb_idl.meter_add.assert_called_once_with(
+            name=self._log_driver.meter_name,
+            unit="pktps",
+            rate=self.fake_cfg_network_log.rate_limit,
+            fair=True,
+            burst_size=self.fake_cfg_network_log.burst_limit,
+            may_exist=False,
+            external_ids={constants.OVN_DEVICE_OWNER_EXT_ID_KEY:
+                          log_const.LOGGING_PLUGIN})
+
+    def test_create_ovn_fair_meter_unchanged(self):
+        mock_find_rows = mock.Mock()
+        mock_find_rows.execute.return_value = [self._fake_meter()]
+        self.nb_idl.db_find_rows.return_value = mock_find_rows
+        self.nb_idl.lookup.side_effect = lambda table, key, default: (
+            self._fake_meter_band() if key == "test_band" else default)
+        self.ovn_client.create_ovn_fair_meter(self._log_driver.meter_name)
+        self.assertFalse(self.nb_idl.meter_del.called)
+        self.assertFalse(self.nb_idl.meter_add.called)
+
+    def test_create_ovn_fair_meter_changed(self):
+        mock_find_rows = mock.Mock()
+        mock_find_rows.execute.return_value = [self._fake_meter(fair=[False])]
+        self.nb_idl.db_find_rows.return_value = mock_find_rows
+        self.nb_idl.lookup.return_value = self._fake_meter_band()
+        self.ovn_client.create_ovn_fair_meter(self._log_driver.meter_name)
+        self.assertTrue(self.nb_idl.meter_del.called)
+        self.assertTrue(self.nb_idl.meter_add.called)
+
+    def test_create_ovn_fair_meter_band_changed(self):
+        mock_find_rows = mock.Mock()
+        mock_find_rows.execute.return_value = [self._fake_meter()]
+        self.nb_idl.db_find_rows.return_value = mock_find_rows
+        self.nb_idl.lookup.return_value = self._fake_meter_band(rate=666)
+        self.ovn_client.create_ovn_fair_meter(self._log_driver.meter_name)
+        self.assertTrue(self.nb_idl.meter_del.called)
+        self.assertTrue(self.nb_idl.meter_add.called)
+
+    def test_create_ovn_fair_meter_band_missing(self):
+        mock_find_rows = mock.Mock()
+        mock_find_rows.execute.return_value = [self._fake_meter()]
+        self.nb_idl.db_find_rows.return_value = mock_find_rows
+        self.nb_idl.lookup.side_effect = None
+        self.ovn_client.create_ovn_fair_meter(self._log_driver.meter_name)
+        self.assertTrue(self.nb_idl.meter_del.called)
+        self.assertTrue(self.nb_idl.meter_add.called)

neutron/tests/unit/services/logapi/drivers/ovn/test_driver.py

@@ -29,7 +29,7 @@
 FAKE_CFG_BURST = 321
 
 
-class TestOVNDriver(base.BaseTestCase):
+class TestOVNDriverBase(base.BaseTestCase):
 
     def setUp(self):
         super().setUp()
@@ -91,6 +91,8 @@ def _fake_meter_band(self, **kwargs):
         meter_band_obj_dict = {**meter_band_defaults_dict, **kwargs}
         return mock.Mock(**meter_band_obj_dict)
 
+
+class TestOVNDriver(TestOVNDriverBase):
     def test_create(self):
         driver = self._log_driver
         self.assertEqual(self.log_plugin, driver._log_plugin)
@@ -106,62 +108,6 @@ def test_create_meter_name(self):
         driver2 = self._log_driver_reinit()
         self.assertEqual(test_log_base, driver2.meter_name)
 
-    def test__create_ovn_fair_meter(self):
-        mock_find_rows = mock.Mock()
-        mock_find_rows.execute.return_value = None
-        self._nb_ovn.db_find_rows.return_value = mock_find_rows
-        self._log_driver._create_ovn_fair_meter(self._nb_ovn.transaction)
-        self.assertFalse(self._nb_ovn.meter_del.called)
-        self.assertTrue(self._nb_ovn.meter_add.called)
-        self.assertFalse(
-            self._nb_ovn.transaction.return_value.__enter__.called)
-        self._nb_ovn.meter_add.assert_called_once_with(
-            name="acl_log_meter",
-            unit="pktps",
-            rate=FAKE_CFG_RATE,
-            fair=True,
-            burst_size=FAKE_CFG_BURST,
-            may_exist=False,
-            external_ids={ovn_const.OVN_DEVICE_OWNER_EXT_ID_KEY:
-                          log_const.LOGGING_PLUGIN})
-
-    def test__create_ovn_fair_meter_unchanged(self):
-        mock_find_rows = mock.Mock()
-        mock_find_rows.execute.return_value = [self._fake_meter()]
-        self._nb_ovn.db_find_rows.return_value = mock_find_rows
-        self._nb_ovn.lookup.side_effect = lambda table, key: (
-            self._fake_meter_band() if key == "test_band" else None)
-        self._log_driver._create_ovn_fair_meter(self._nb_ovn.transaction)
-        self.assertFalse(self._nb_ovn.meter_del.called)
-        self.assertFalse(self._nb_ovn.meter_add.called)
-
-    def test__create_ovn_fair_meter_changed(self):
-        mock_find_rows = mock.Mock()
-        mock_find_rows.execute.return_value = [self._fake_meter(fair=[False])]
-        self._nb_ovn.db_find_rows.return_value = mock_find_rows
-        self._nb_ovn.lookup.return_value = self._fake_meter_band()
-        self._log_driver._create_ovn_fair_meter(self._nb_ovn.transaction)
-        self.assertTrue(self._nb_ovn.meter_del.called)
-        self.assertTrue(self._nb_ovn.meter_add.called)
-
-    def test__create_ovn_fair_meter_band_changed(self):
-        mock_find_rows = mock.Mock()
-        mock_find_rows.execute.return_value = [self._fake_meter()]
-        self._nb_ovn.db_find_rows.return_value = mock_find_rows
-        self._nb_ovn.lookup.return_value = self._fake_meter_band(rate=666)
-        self._log_driver._create_ovn_fair_meter(self._nb_ovn.transaction)
-        self.assertTrue(self._nb_ovn.meter_del.called)
-        self.assertTrue(self._nb_ovn.meter_add.called)
-
-    def test__create_ovn_fair_meter_band_missing(self):
-        mock_find_rows = mock.Mock()
-        mock_find_rows.execute.return_value = [self._fake_meter()]
-        self._nb_ovn.db_find_rows.return_value = mock_find_rows
-        self._nb_ovn.lookup.side_effect = idlutils.RowNotFound
-        self._log_driver._create_ovn_fair_meter(self._nb_ovn.transaction)
-        self.assertTrue(self._nb_ovn.meter_del.called)
-        self.assertTrue(self._nb_ovn.meter_add.called)
-
     class _fake_acl():
         def __init__(self, name=None, **acl_dict):
             acl_defaults_dict = {