[OVN] Update ovn meter when neutron server reloadsstable/wallaby

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
    neutron/services/logapi/drivers/ovn/driver.py

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

6 files changed, 212 insertions, 58 deletions

diff --git a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/maintenance.py b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/maintenance.py
index 4f731fe8c8..b781e9ae9f 100644
--- a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/maintenance.py
+++ b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/maintenance.py
@@ -42,6 +42,7 @@ from neutron.db import ovn_revision_numbers_db as revision_numbers_db
 from neutron.db import segments_db
 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
@@ -966,6 +967,24 @@ class DBInconsistenciesPeriodics(SchemaAwarePeriodicsBase):
 
         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):
 
diff --git a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_client.py b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_client.py
index b7f216a75e..e84ed5fe7a 100644
--- a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_client.py
+++ b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_client.py
@@ -30,6 +30,7 @@ from neutron_lib.exceptions import l3 as l3_exc
 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
@@ -2494,3 +2495,54 @@ class OVNClient(object):
             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)
diff --git a/neutron/services/logapi/drivers/ovn/driver.py b/neutron/services/logapi/drivers/ovn/driver.py
index 7ab1913a58..bfd517e7f2 100644
--- a/neutron/services/logapi/drivers/ovn/driver.py
+++ b/neutron/services/logapi/drivers/ovn/driver.py
@@ -90,6 +90,10 @@ class OVNDriver(base.DriverBase):
         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
 
@@ -301,7 +305,8 @@ class OVNDriver(base.DriverBase):
         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))
 
diff --git a/neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_maintenance.py b/neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_maintenance.py
index 5c37a6f263..b7adbab647 100644
--- a/neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_maintenance.py
+++ b/neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_maintenance.py
@@ -30,9 +30,15 @@ from neutron.conf.plugins.ml2.drivers.ovn import ovn_conf as ovn_config
 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."""
@@ -1043,3 +1049,37 @@ class TestMaintenance(_TestMaintenanceHelper):
 
             # 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)
diff --git a/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_ovn_client.py b/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_ovn_client.py
new file mode 100644
index 0000000000..11a17657c3
--- /dev/null
+++ b/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_ovn_client.py
@@ -0,0 +1,92 @@
+# Copyright 2022 Canonical
+# All Rights Reserved.
+#
+#    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.
+
+from unittest import mock
+
+from neutron.common.ovn import constants
+from neutron.conf.plugins.ml2.drivers.ovn import ovn_conf
+from neutron.plugins.ml2.drivers.ovn.mech_driver.ovsdb import ovn_client
+from neutron.tests import base
+from neutron.tests.unit.services.logapi.drivers.ovn \
+    import test_driver as test_log_driver
+from neutron_lib.services.logapi import constants as log_const
+
+
+class TestOVNClientBase(base.BaseTestCase):
+
+    def setUp(self):
+        ovn_conf.register_opts()
+        super(TestOVNClientBase, self).setUp()
+        self.nb_idl = mock.MagicMock()
+        self.sb_idl = mock.MagicMock()
+        self.ovn_client = ovn_client.OVNClient(self.nb_idl, self.sb_idl)
+
+
+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)
diff --git a/neutron/tests/unit/services/logapi/drivers/ovn/test_driver.py b/neutron/tests/unit/services/logapi/drivers/ovn/test_driver.py
index 35c2ec5765..4f4cd7a7ba 100644
--- a/neutron/tests/unit/services/logapi/drivers/ovn/test_driver.py
+++ b/neutron/tests/unit/services/logapi/drivers/ovn/test_driver.py
@@ -29,7 +29,7 @@ FAKE_CFG_RATE = 123
 FAKE_CFG_BURST = 321
 
 
-class TestOVNDriver(base.BaseTestCase):
+class TestOVNDriverBase(base.BaseTestCase):
 
     def setUp(self):
         super().setUp()
@@ -91,6 +91,8 @@ class TestOVNDriver(base.BaseTestCase):
         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 @@ class TestOVNDriver(base.BaseTestCase):
         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 = {