From c3602ac19b62b77d3cb763746e124881d4c061e4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Elvira=20Garc=C3=ADa?= Date: Thu, 20 Apr 2023 16:39:12 +0200 Subject: [PATCH] [OVN] Update ovn meter when neutron server reloads MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. Closes-bug: #2017145 Signed-off-by: Elvira García Change-Id: I24cef85ed68c893a740445707f88296d763c8de8 --- .../ovn/mech_driver/ovsdb/maintenance.py | 19 ++++++ .../ovn/mech_driver/ovsdb/ovn_client.py | 52 ++++++++++++++++ neutron/services/logapi/drivers/ovn/driver.py | 51 ++-------------- .../ovn/mech_driver/ovsdb/test_maintenance.py | 40 ++++++++++++ .../ovn/mech_driver/ovsdb/test_ovn_client.py | 61 +++++++++++++++++++ .../logapi/drivers/ovn/test_driver.py | 60 +----------------- 6 files changed, 181 insertions(+), 102 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 0f3deed9a19..7f4b15c0eee 100644 --- a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/maintenance.py +++ b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/maintenance.py @@ -41,6 +41,7 @@ from neutron.db import ovn_revision_numbers_db as revision_numbers_db 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 @@ -1016,6 +1017,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 11650493efe..9a40a568f2d 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 @@ -31,6 +31,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 @@ -2647,3 +2648,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 eeca2f18d38..8a4c8d2de6a 100644 --- a/neutron/services/logapi/drivers/ovn/driver.py +++ b/neutron/services/logapi/drivers/ovn/driver.py @@ -89,54 +89,14 @@ class OVNDriver(base.DriverBase): 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 @@ 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 a4111d453aa..6cd025045f4 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.""" @@ -942,3 +948,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 index 9e6adb8bc92..d0f2eb599fe 100644 --- 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 @@ -21,9 +21,12 @@ 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 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 l3 from neutron_lib.api.definitions import portbindings from neutron_lib import constants as const +from neutron_lib.services.logapi import constants as log_const class TestOVNClientBase(base.BaseTestCase): @@ -191,3 +194,61 @@ class TestOVNClientDetermineBindHost(TestOVNClientBase): 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) 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 35c2ec5765a..4f4cd7a7ba1 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 = {