From b922e6fd76a01b51163a6a2eea3372e7fc3011db Mon Sep 17 00:00:00 2001 From: Rodolfo Alonso Hernandez Date: Tue, 25 Mar 2025 18:32:43 +0000 Subject: [PATCH 1/8] HA_Chassis_Group create command with HA_Chassis assignation This new command allows to create a "HA_Chassis_Group" register and the corresponding "HA_Chassis" child registers. If the parent "HA_Chassis_Group" register already exists, it allows to update the priority of the "HA_Chassis" registers or deleting the discarded ones. Partial-Bug: #2092271 Change-Id: I73ed014ae6a36e171c1eace8bea45010cf8415fa Conflicts: neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/commands.py --- .../drivers/ovn/mech_driver/ovsdb/commands.py | 64 ++++++++++++++ .../ovn/mech_driver/ovsdb/impl_idl_ovn.py | 6 ++ .../ovn/mech_driver/ovsdb/test_impl_idl.py | 84 +++++++++++++++++++ 3 files changed, 154 insertions(+) diff --git a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/commands.py b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/commands.py index 687db6bd4c1..ef552c13040 100644 --- a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/commands.py +++ b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/commands.py @@ -13,6 +13,8 @@ # under the License. import abc +import copy +import uuid from oslo_utils import timeutils from ovsdbapp.backend.ovs_idl import command @@ -100,6 +102,49 @@ def _add_gateway_chassis(api, txn, lrp_name, val): return 'gateway_chassis', uuid_list +def _sync_ha_chassis_group(txn, nb_api, name, chassis_priority, + may_exist=False, table_name='HA_Chassis_Group', + **columns): + result = None + hcg = nb_api.lookup(table_name, name, default=None) + if hcg: + if not may_exist: + raise RuntimeError(_('HA_Chassis_Group %s exists' % name)) + else: + hcg = txn.insert(nb_api._tables[table_name]) + hcg.name = name + command.BaseCommand.set_columns(hcg, **columns) + result = hcg.uuid + + # HA_Chassis registers handling. + # Remove the non-existing chassis in ``self.chassis_priority`` + hc_to_remove = [] + for hc in getattr(hcg, 'ha_chassis', []): + if hc.chassis_name not in chassis_priority: + hc_to_remove.append(hc) + + for hc in hc_to_remove: + hcg.delvalue('ha_chassis', hc) + hc.delete() + + # Update the priority of the existing chassis. + for hc in getattr(hcg, 'ha_chassis', []): + hc_priority = chassis_priority.pop(hc.chassis_name) + hc.priority = hc_priority + + # Add the non-existing HA_Chassis registers. + for hc_name, priority in chassis_priority.items(): + hc = txn.insert(nb_api.tables['HA_Chassis']) + hc.chassis_name = hc_name + hc.priority = priority + hcg.addvalue('ha_chassis', hc) + + if not result: + result = rowview.RowView(hcg) + + return result + + class CheckLivenessCommand(command.BaseCommand): def run_idl(self, txn): # txn.pre_commit responsible for updating nb_global.nb_cfg, but @@ -1089,3 +1134,22 @@ def run_idl(self, txn): virtual_parents) setattr(lsp, 'options', options) + + +class HAChassisGroupWithHCAddCommand(command.AddCommand): + table_name = 'HA_Chassis_Group' + + def __init__(self, api, name, chassis_priority, may_exist=False, + **columns): + super().__init__(api) + self.name = name + self.chassis_priority = copy.deepcopy(chassis_priority) + self.may_exist = may_exist + self.columns = columns + + def run_idl(self, txn): + # HA_Chassis_Group register creation. + self.result = _sync_ha_chassis_group( + txn, self.api, self.name, self.chassis_priority, + may_exist=self.may_exist, table_name=self.table_name, + **self.columns) diff --git a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/impl_idl_ovn.py b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/impl_idl_ovn.py index 7c25b69049e..358048059c6 100644 --- a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/impl_idl_ovn.py +++ b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/impl_idl_ovn.py @@ -906,6 +906,12 @@ def set_router_mac_age_limit(self, router=None): return cmd.SetLRouterMacAgeLimitCommand( self, router, cfg.get_ovn_mac_binding_age_threshold()) + def ha_chassis_group_with_hc_add(self, name, chassis_priority, + may_exist=False, **columns): + return cmd.HAChassisGroupWithHCAddCommand( + self, name, chassis_priority, may_exist=may_exist, + **columns) + class OvsdbSbOvnIdl(sb_impl_idl.OvnSbApiIdlImpl, Backend): @n_utils.classproperty diff --git a/neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_impl_idl.py b/neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_impl_idl.py index f5adcbea58a..d4d2c20a74c 100644 --- a/neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_impl_idl.py +++ b/neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_impl_idl.py @@ -12,6 +12,7 @@ # under the License. # +from collections import abc import copy from unittest import mock import uuid @@ -695,6 +696,89 @@ def test_modify_static_route_external_ids(self): self.assertEqual(external_ids, lr.static_routes[0].external_ids) + def _cleanup_delete_hcg(self, hcg_name): + if isinstance(hcg_name, str): + self.nbapi.db_destroy('HA_Chassis_Group', hcg_name).execute( + check_error=True) + elif isinstance(hcg_name, abc.Iterable): + for _hcg_name in hcg_name: + self.nbapi.db_destroy('HA_Chassis_Group', _hcg_name).execute( + check_error=True) + + def _check_hcg(self, hcg, hcg_name, chassis_priority, + chassis_priority_deleted=None): + self.assertEqual(hcg_name, hcg.name) + self.assertEqual(len(chassis_priority), len(hcg.ha_chassis)) + for hc in hcg.ha_chassis: + self.assertEqual(chassis_priority[hc.chassis_name], hc.priority) + + if chassis_priority_deleted: + for hc_name in chassis_priority_deleted: + self.assertIsNone( + self.nbapi.lookup('HA_Chassis', hc_name, default=None)) + + def test_ha_chassis_group_with_hc_add_no_existing_hcg(self): + chassis_priority = {'ch1': 1, 'ch2': 2, 'ch3': 3, 'ch4': 4} + hcg_name = uuidutils.generate_uuid() + self.addCleanup(self._cleanup_delete_hcg, hcg_name) + hcg = self.nbapi.ha_chassis_group_with_hc_add( + hcg_name, chassis_priority).execute(check_error=True) + self._check_hcg(hcg, hcg_name, chassis_priority) + + def test_ha_chassis_group_with_hc_add_existing_hcg(self): + chassis_priority = {'ch1': 1, 'ch2': 2, 'ch3': 3, 'ch4': 4} + hcg_name = uuidutils.generate_uuid() + self.addCleanup(self._cleanup_delete_hcg, hcg_name) + self.nbapi.ha_chassis_group_with_hc_add( + hcg_name, chassis_priority).execute(check_error=True) + cmd = self.nbapi.ha_chassis_group_with_hc_add( + hcg_name, chassis_priority) + self.assertRaises(RuntimeError, cmd.execute, check_error=True) + + def test_ha_chassis_group_with_hc_add_existing_hcg_may_exist(self): + chassis_priority = {'ch1': 1, 'ch2': 2, 'ch3': 3, 'ch4': 4} + hcg_name = uuidutils.generate_uuid() + self.addCleanup(self._cleanup_delete_hcg, hcg_name) + hcg = None + for _ in range(2): + hcg = self.nbapi.ha_chassis_group_with_hc_add( + hcg_name, chassis_priority, may_exist=True).execute( + check_error=True) + self._check_hcg(hcg, hcg_name, chassis_priority) + + def test_ha_chassis_group_with_hc_add_existing_hcg_update_chassis(self): + # This test: + # - adds new chassis: ch5, ch6 + # - removes others: ch3, ch4 + # - changes the priority of the existing ones ch1, ch2 + chassis_priority = {'ch1': 1, 'ch2': 2, 'ch3': 3, 'ch4': 4} + hcg_name = uuidutils.generate_uuid() + self.addCleanup(self._cleanup_delete_hcg, hcg_name) + self.nbapi.ha_chassis_group_with_hc_add( + hcg_name, chassis_priority).execute(check_error=True) + + chassis_priority = {'ch1': 2, 'ch2': 1, 'ch5': 3, 'ch6': 4} + hcg = self.nbapi.ha_chassis_group_with_hc_add( + hcg_name, chassis_priority, may_exist=True).execute( + check_error=True) + self._check_hcg(hcg, hcg_name, chassis_priority, + chassis_priority_deleted=['ch3', 'ch4']) + + def test_ha_chassis_group_with_hc_add_two_hcg(self): + # Both HCG will have the same chassis priority (the same chassis + # names, that is something very common. + chassis_priority1 = {'ch1': 1, 'ch2': 2, 'ch3': 3, 'ch4': 4} + chassis_priority2 = {'ch1': 11, 'ch2': 12, 'ch3': 13, 'ch4': 14} + hcg_name1 = uuidutils.generate_uuid() + hcg_name2 = uuidutils.generate_uuid() + self.addCleanup(self._cleanup_delete_hcg, [hcg_name1, hcg_name2]) + hcg1 = self.nbapi.ha_chassis_group_with_hc_add( + hcg_name1, chassis_priority1).execute(check_error=True) + hcg2 = self.nbapi.ha_chassis_group_with_hc_add( + hcg_name2, chassis_priority2).execute(check_error=True) + self._check_hcg(hcg1, hcg_name1, chassis_priority1) + self._check_hcg(hcg2, hcg_name2, chassis_priority2) + class TestIgnoreConnectionTimeout(BaseOvnIdlTest): @classmethod From d500101acfc0e73befbf853772eaf1b180b59a4d Mon Sep 17 00:00:00 2001 From: Rodolfo Alonso Hernandez Date: Thu, 27 Mar 2025 03:01:35 +0000 Subject: [PATCH 2/8] [OVN] Add a Logical_Router_Port HA_Chassis retrieval method This method returns the name of the chassis assigned in the "HA_Chassis_Group" associated with the "Logical_Router_Port", ordered by the priority. Partial-Bug: #2092271 Change-Id: Ifbecf0f2699297a85e67299ce6d8ec98fd41fae4 Conflicts: neutron/common/ovn/utils.py --- neutron/common/ovn/utils.py | 38 ++++++++++ .../tests/functional/common/ovn/test_utils.py | 74 +++++++++++++++++++ 2 files changed, 112 insertions(+) diff --git a/neutron/common/ovn/utils.py b/neutron/common/ovn/utils.py index e88a3d89149..1432072205d 100644 --- a/neutron/common/ovn/utils.py +++ b/neutron/common/ovn/utils.py @@ -1377,3 +1377,41 @@ def validate_port_forwarding_configuration(): if any(net_type in provider_network_types for net_type in cfg.CONF.ml2.tenant_network_types): raise ovn_exc.InvalidPortForwardingConfiguration() + + +def ovs_persist_uuid_supported(nb_idl): + # OVS 3.1+ contain the persist_uuid feature that allows choosing the UUID + # that will be stored in the DB. It was broken prior to 3.1.5/3.2.3/3.3.1 + # so this will return True only for the fixed version. As actually testing + # the fix requires committing a transaction, an implementation detail is + # tested. This can be removed once a fixed version is required. + global _OVS_PERSIST_UUID + if _OVS_PERSIST_UUID is _SENTINEL: + _OVS_PERSIST_UUID = isinstance( + next(iter(nb_idl.tables["NB_Global"].rows.data.values())), list) + LOG.debug(f"OVS persist_uuid supported={_OVS_PERSIST_UUID}") + return _OVS_PERSIST_UUID + + +def get_logical_router_port_ha_chassis(nb_idl, lrp, priorities=None): + """Get the list of chassis hosting this Logical_Router_Port. + + :param nb_idl: (``OvsdbNbOvnIdl``) OVN Northbound IDL + :param lrp: Logical_Router_Port + :param priorities: (list of int) a list of HA_Chassis chassis priorities + to search for + :return: List of tuples (chassis_name, priority) sorted by priority. If + ``priorities`` is set then only chassis matching of these + priorities are returned. + """ + chassis = [] + lrp = nb_idl.lookup('Logical_Router_Port', lrp.name, default=None) + if not lrp or not lrp.ha_chassis_group: + return chassis + + for hc in lrp.ha_chassis_group[0].ha_chassis: + if priorities and hc.priority not in priorities: + continue + chassis.append((hc.chassis_name, hc.priority)) + + return chassis diff --git a/neutron/tests/functional/common/ovn/test_utils.py b/neutron/tests/functional/common/ovn/test_utils.py index ffdd7816e3d..7f371ed882e 100644 --- a/neutron/tests/functional/common/ovn/test_utils.py +++ b/neutron/tests/functional/common/ovn/test_utils.py @@ -13,6 +13,7 @@ # under the License. import ddt +from neutron_lib.api.definitions import external_net from neutron_lib.api.definitions import portbindings from oslo_utils import uuidutils from ovsdbapp.backend.ovs_idl import event @@ -321,3 +322,76 @@ def test_without_transaction(self, method, _args, _kwargs): def test_needed_parameters(self, method): self.assertRaises(RuntimeError, method, uuidutils.generate_uuid(), None, None) + + +class TestGetLogicalRouterPortHAChassis(base.TestOVNFunctionalBase): + def _create_network_and_port(self): + kwargs = {external_net.EXTERNAL: True, 'as_admin': True} + net = self._make_network(self.fmt, 'n1', True, **kwargs)['network'] + port_data = {'port': {'network_id': net['id'], + 'tenant_id': self._tenant_id,}} + port_req = self.new_create_request('ports', port_data, self.fmt) + port_res = port_req.get_response(self.api) + return self.deserialize(self.fmt, port_res)['port'] + + def _create_gw_chassis(self, num_chassis): + chassis = [] + for _ in range(num_chassis): + chassis.append(self.add_fake_chassis( + uuidutils.generate_uuid(), azs=[], + enable_chassis_as_gw=True)) + return chassis + + def _create_router(self, network_id): + gw_info = {'network_id': network_id} + router = {'router': {'name': uuidutils.generate_uuid(), + 'admin_state_up': True, + 'tenant_id': self._tenant_id, + 'external_gateway_info': gw_info}} + return self.l3_plugin.create_router(self.context, router) + + def _set_lrp_hcg(self, gw_port_id, hcg): + lrp_name = utils.ovn_lrouter_port_name(gw_port_id) + self.nb_api.db_set( + 'Logical_Router_Port', lrp_name, + ('ha_chassis_group', hcg.uuid)).execute() + return self.nb_api.lookup('Logical_Router_Port', lrp_name) + + def _get_router_hcg(self, router_id): + hcg_name = utils.ovn_name(router_id) + return self.nb_api.lookup('HA_Chassis_Group', hcg_name) + + def _check_chassis(self, ha_chassis, expected_chassis, priorities=None): + length = len(priorities) if priorities else len(expected_chassis) + self.assertEqual(length, len(ha_chassis)) + ch_priorities = set([]) + for hc in ha_chassis: + self.assertIn(hc[0], expected_chassis) + ch_priorities.add(hc[1]) + self.assertEqual(length, len(ch_priorities)) + if priorities: + for ch_priority in ch_priorities: + self.assertIn(ch_priority, priorities) + + def test_get_ha_chassis(self): + port = self._create_network_and_port() + ch_list = self._create_gw_chassis(5) + router = self._create_router(port['network_id']) + hcg = self._get_router_hcg(router['id']) + lrp = self._set_lrp_hcg(router['gw_port_id'], hcg) + + ha_chassis = utils.get_logical_router_port_ha_chassis(self.nb_api, lrp) + self._check_chassis(ha_chassis, ch_list) + + def test_get_ha_chassis_priorities(self): + port = self._create_network_and_port() + ch_list = self._create_gw_chassis(5) + router = self._create_router(port['network_id']) + hcg = self._get_router_hcg(router['id']) + lrp = self._set_lrp_hcg(router['gw_port_id'], hcg) + + prio = [ovn_const.HA_CHASSIS_GROUP_HIGHEST_PRIORITY, + ovn_const.HA_CHASSIS_GROUP_HIGHEST_PRIORITY - 1] + ha_chassis = utils.get_logical_router_port_ha_chassis( + self.nb_api, lrp, priorities=prio) + self._check_chassis(ha_chassis, ch_list, priorities=prio) From f493715dfa65d4018b6a15fc90de5605310a2f78 Mon Sep 17 00:00:00 2001 From: Rodolfo Alonso Hernandez Date: Wed, 16 Apr 2025 07:21:37 +0000 Subject: [PATCH 3/8] Update ``filter_existing_chassis`` signature and make it static Partial-Bug: #2092271 Change-Id: Iafd1edcda72720bcd185d9d232de8e8e03a7cbff --- .../ml2/drivers/ovn/mech_driver/ovsdb/commands.py | 3 +-- neutron/scheduler/l3_ovn_scheduler.py | 4 ++-- .../tests/unit/scheduler/test_l3_ovn_scheduler.py | 13 +++++-------- 3 files changed, 8 insertions(+), 12 deletions(-) diff --git a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/commands.py b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/commands.py index ef552c13040..0fc0ddd450d 100644 --- a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/commands.py +++ b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/commands.py @@ -392,8 +392,7 @@ def run_idl(self, txn): az_hints = self.api.get_gateway_chassis_az_hints(self.g_name) filtered_existing_chassis = ( self.scheduler.filter_existing_chassis( - nb_idl=self.api, gw_chassis=self.all_gw_chassis, - physnet=physnet, + gw_chassis=self.all_gw_chassis, physnet=physnet, chassis_physnets=self.chassis_with_physnets, existing_chassis=existing_chassis, az_hints=az_hints, chassis_with_azs=self.chassis_with_azs)) diff --git a/neutron/scheduler/l3_ovn_scheduler.py b/neutron/scheduler/l3_ovn_scheduler.py index f18a627980d..1c21ed9e18a 100644 --- a/neutron/scheduler/l3_ovn_scheduler.py +++ b/neutron/scheduler/l3_ovn_scheduler.py @@ -40,8 +40,8 @@ def select(self, nb_idl, sb_idl, gateway_name, candidates=None, scheduled. """ - def filter_existing_chassis(self, nb_idl, gw_chassis, - physnet, chassis_physnets, + @staticmethod + def filter_existing_chassis(gw_chassis, physnet, chassis_physnets, existing_chassis, az_hints, chassis_with_azs): chassis_list = copy.copy(existing_chassis) for chassis_name in existing_chassis or []: diff --git a/neutron/tests/unit/scheduler/test_l3_ovn_scheduler.py b/neutron/tests/unit/scheduler/test_l3_ovn_scheduler.py index a7aff4086d8..e4d64df7567 100644 --- a/neutron/tests/unit/scheduler/test_l3_ovn_scheduler.py +++ b/neutron/tests/unit/scheduler/test_l3_ovn_scheduler.py @@ -118,8 +118,7 @@ def select(self, chassis_gateway_mapping, gateway_name, def filter_existing_chassis(self, *args, **kwargs): return self.l3_scheduler.filter_existing_chassis( - nb_idl=kwargs.pop('nb_idl'), gw_chassis=kwargs.pop('gw_chassis'), - physnet=kwargs.pop('physnet'), + gw_chassis=kwargs.pop('gw_chassis'), physnet=kwargs.pop('physnet'), chassis_physnets=kwargs.pop('chassis_physnets'), existing_chassis=kwargs.pop('existing_chassis'), az_hints=kwargs.pop('az_hints', []), @@ -171,26 +170,24 @@ def test_filter_existing_chassis(self): # it from Base class didnt seem right. Also, there is no need to have # another test in LeastLoadedScheduler. chassis_physnets = {'temp': ['phys-network-0', 'phys-network-1']} - nb_idl = FakeOVNGatewaySchedulerNbOvnIdl( - self.fake_chassis_gateway_mappings['None'], 'g1') # Check if invalid chassis is removed self.assertEqual( ['temp'], self.filter_existing_chassis( - nb_idl=nb_idl, gw_chassis=["temp"], + gw_chassis=["temp"], physnet='phys-network-1', chassis_physnets=chassis_physnets, existing_chassis=['temp', None])) # Check if invalid is removed -II self.assertFalse( self.filter_existing_chassis( - nb_idl=nb_idl, gw_chassis=["temp"], + gw_chassis=["temp"], physnet='phys-network-1', chassis_physnets=chassis_physnets, existing_chassis=None)) # Check if chassis removed when physnet doesnt exist self.assertFalse( self.filter_existing_chassis( - nb_idl=nb_idl, gw_chassis=["temp"], + gw_chassis=["temp"], physnet='phys-network-2', chassis_physnets=chassis_physnets, existing_chassis=['temp'])) @@ -198,7 +195,7 @@ def test_filter_existing_chassis(self): # or in chassis_physnets self.assertFalse( self.filter_existing_chassis( - nb_idl=nb_idl, gw_chassis=["temp1"], + gw_chassis=["temp1"], physnet='phys-network-2', chassis_physnets=chassis_physnets, existing_chassis=['temp'])) From a2235ac5659bda92b446a3cc93119d12d55506b7 Mon Sep 17 00:00:00 2001 From: Rodolfo Alonso Hernandez Date: Mon, 21 Apr 2025 14:42:52 +0000 Subject: [PATCH 4/8] [OVN] Method to retrieve the LRP chassis list Added a new method in ``OvsdbNbOvnIdl`` to retrieve the gateway ``Logical_Router_Port`` chassis list with priorities, stored in the ``HA_Chassis_Group`` register associated to the router. Partial-Bug: #2092271 Change-Id: I6b2bc7a3e80a906f146da3645c530d175f31a8dd --- .../ovn/mech_driver/ovsdb/impl_idl_ovn.py | 24 +++++++++++ .../ovn/mech_driver/ovsdb/test_impl_idl.py | 40 +++++++++++++++++++ 2 files changed, 64 insertions(+) diff --git a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/impl_idl_ovn.py b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/impl_idl_ovn.py index 358048059c6..3f8885f15bd 100644 --- a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/impl_idl_ovn.py +++ b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/impl_idl_ovn.py @@ -520,6 +520,30 @@ def _get_logical_router_port_gateway_chassis(self, lrp, priorities=None): # make sure that chassis are sorted by priority return sorted(chassis, reverse=True, key=lambda x: x[1]) + @staticmethod + def _get_logical_router_port_ha_chassis_group(lrp, priorities=None): + """Get the list of chassis hosting this gateway port. + + @param lrp: logical router port + @type lrp: Logical_Router_Port row + @param priorities: a list of gateway chassis priorities to search for + @type priorities: list of int + @return: List of tuples (chassis_name, priority) sorted by priority. If + ``priorities`` is set then only chassis matching of these + priorities are returned. + """ + chassis = [] + hcg = getattr(lrp, 'ha_chassis_group', None) + if not hcg: + return chassis + + for hc in hcg[0].ha_chassis: + if priorities is not None and hc.priority not in priorities: + continue + chassis.append((hc.chassis_name, hc.priority)) + # Make sure that chassis are sorted by priority (highest prio first) + return sorted(chassis, reverse=True, key=lambda x: x[1]) + def get_all_chassis_gateway_bindings(self, chassis_candidate_list=None, priorities=None): diff --git a/neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_impl_idl.py b/neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_impl_idl.py index d4d2c20a74c..3b441f2b123 100644 --- a/neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_impl_idl.py +++ b/neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_impl_idl.py @@ -19,6 +19,7 @@ import netaddr from neutron_lib import constants +from neutron_lib.utils import net as net_utils from oslo_utils import netutils from oslo_utils import uuidutils from ovsdbapp.backend.ovs_idl import connection @@ -779,6 +780,45 @@ def test_ha_chassis_group_with_hc_add_two_hcg(self): self._check_hcg(hcg1, hcg_name1, chassis_priority1) self._check_hcg(hcg2, hcg_name2, chassis_priority2) + def _add_lrp_with_gw(self, chassis_priority=None, is_gw=True): + if is_gw: + hcg_name = uuidutils.generate_uuid() + hcg = self.nbapi.ha_chassis_group_with_hc_add( + hcg_name, chassis_priority).execute(check_error=True) + kwargs = {'ha_chassis_group': hcg.uuid} + else: + hcg = None + kwargs = {} + + mac = next(net_utils.random_mac_generator(['ca', 'fe', 'ca', 'fe'])) + networks = ['192.0.2.0/24'] + lr = self.nbapi.lr_add(uuidutils.generate_uuid()).execute( + check_error=True) + + lrp = self.nbapi.lrp_add( + lr.uuid, uuidutils.generate_uuid(), mac, networks, + **kwargs).execute(check_error=True) + return lr, lrp, hcg + + def test__get_logical_router_port_ha_chassis_group(self): + chassis_priority = {'ch1': 1, 'ch2': 2, 'ch3': 3, 'ch4': 4} + lr, lrp, hcg = self._add_lrp_with_gw(chassis_priority) + cprio_res = self.nbapi._get_logical_router_port_ha_chassis_group(lrp) + self.assertEqual([('ch4', 4), ('ch3', 3), ('ch2', 2), ('ch1', 1)], + cprio_res) + + def test__get_logical_router_port_ha_chassis_group_with_priorities(self): + chassis_priority = {'ch1': 1, 'ch2': 2, 'ch3': 3, 'ch4': 4} + lr, lrp, hcg = self._add_lrp_with_gw(chassis_priority) + cprio_res = self.nbapi._get_logical_router_port_ha_chassis_group( + lrp, priorities=(1, 3, 4)) + self.assertEqual([('ch4', 4), ('ch3', 3), ('ch1', 1)], cprio_res) + + def test__get_logical_router_port_ha_chassis_group_no_hcg(self): + lr, lrp, hcg = self._add_lrp_with_gw(is_gw=False) + cprio_res = self.nbapi._get_logical_router_port_ha_chassis_group(lrp) + self.assertEqual([], cprio_res) + class TestIgnoreConnectionTimeout(BaseOvnIdlTest): @classmethod From 772ad876b0da7c34a88ac53c5894f0ed5c87bb21 Mon Sep 17 00:00:00 2001 From: Rodolfo Alonso Hernandez Date: Tue, 22 Apr 2025 23:51:49 +0000 Subject: [PATCH 5/8] [OVN] Handle HA_Chassis_Group field in LRP creation and update Now is possible to set a ``HA_Chassis_Group`` in a ``Logical_Router_Port``, using the NB API methods ``add_lrouter_port`` and ``update_lrouter_port``. Both methods support creating the ``HA_Chassis_Group`` in the same transation the LRP is created or updated. Partial-Bug: #2092271 Change-Id: Iab7581d2f4eff8ab84a2a379499490353fbe1971 --- .../drivers/ovn/mech_driver/ovsdb/commands.py | 5 +- .../ovn/mech_driver/ovsdb/test_impl_idl.py | 76 +++++++++++++++++++ 2 files changed, 79 insertions(+), 2 deletions(-) diff --git a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/commands.py b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/commands.py index 0fc0ddd450d..f4bccdb6747 100644 --- a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/commands.py +++ b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/commands.py @@ -509,8 +509,9 @@ def run_idl(self, txn): if col == 'gateway_chassis': col, val = _add_gateway_chassis(self.api, txn, self.name, val) - setattr(lrouter_port, col, val) + self.set_column(lrouter_port, col, val) _addvalue_to_list(lrouter, 'ports', lrouter_port) + self.result = lrouter_port.uuid class UpdateLRouterPortCommand(command.BaseCommand): @@ -535,7 +536,7 @@ def run_idl(self, txn): if col == 'gateway_chassis': col, val = _add_gateway_chassis(self.api, txn, self.name, val) - setattr(lrouter_port, col, val) + self.set_column(lrouter_port, col, val) class DelLRouterPortCommand(command.BaseCommand): diff --git a/neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_impl_idl.py b/neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_impl_idl.py index 3b441f2b123..40fb6049d66 100644 --- a/neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_impl_idl.py +++ b/neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_impl_idl.py @@ -819,6 +819,82 @@ def test__get_logical_router_port_ha_chassis_group_no_hcg(self): cprio_res = self.nbapi._get_logical_router_port_ha_chassis_group(lrp) self.assertEqual([], cprio_res) + def test_create_lrp_with_ha_chassis_group_same_txn(self): + mac = next(net_utils.random_mac_generator(['ca', 'fe', 'ca', 'fe'])) + networks = ['192.0.2.0/24'] + lr_name = uuidutils.generate_uuid() + lrp_name = uuidutils.generate_uuid() + self.nbapi.lr_add(lr_name).execute(check_error=True) + + # Create the HCG and the LRP in the same transaction. + with self.nbapi.transaction(check_error=True) as txn: + hcg_cmd = txn.add(self.nbapi.ha_chassis_group_with_hc_add( + uuidutils.generate_uuid(), {'ch1': 1, 'ch2': 2})) + txn.add(self.nbapi.add_lrouter_port( + lrp_name, lr_name, mac=mac, networks=networks, + ha_chassis_group=hcg_cmd)) + + lrp = self.nbapi.lrp_get(lrp_name).execute(check_error=True) + self.assertEqual(hcg_cmd.result.uuid, lrp.ha_chassis_group[0].uuid) + + def test_create_lrp_with_ha_chassis_group_different_txn(self): + mac = next(net_utils.random_mac_generator(['ca', 'fe', 'ca', 'fe'])) + networks = ['192.0.2.0/24'] + lr_name = uuidutils.generate_uuid() + lrp_name = uuidutils.generate_uuid() + self.nbapi.lr_add(lr_name).execute(check_error=True) + + # Create the HCG and the LRP in two consecutive transactions. + hcg = self.nbapi.ha_chassis_group_with_hc_add( + uuidutils.generate_uuid(), {'ch1': 1, 'ch2': 2}).execute( + check_error=True) + self.nbapi.add_lrouter_port( + lrp_name, lr_name, mac=mac, networks=networks, + ha_chassis_group=hcg.uuid).execute(check_error=True) + + lrp = self.nbapi.lrp_get(lrp_name).execute(check_error=True) + self.assertEqual(hcg.uuid, lrp.ha_chassis_group[0].uuid) + + def test_update_lrp_with_ha_chassis_group_same_txn(self): + mac = next(net_utils.random_mac_generator(['ca', 'fe', 'ca', 'fe'])) + networks = ['192.0.2.0/24'] + lr_name = uuidutils.generate_uuid() + lrp_name = uuidutils.generate_uuid() + self.nbapi.lr_add(lr_name).execute(check_error=True) + self.nbapi.add_lrouter_port( + lrp_name, lr_name, mac=mac, + networks=networks).execute(check_error=True) + + # Create the HCG and update the LRP in the same transaction. + with self.nbapi.transaction(check_error=True) as txn: + hcg_cmd = txn.add(self.nbapi.ha_chassis_group_with_hc_add( + uuidutils.generate_uuid(), {'ch1': 1, 'ch2': 2})) + txn.add(self.nbapi.update_lrouter_port( + lrp_name, ha_chassis_group=hcg_cmd)) + + lrp = self.nbapi.lrp_get(lrp_name).execute(check_error=True) + self.assertEqual(hcg_cmd.result.uuid, lrp.ha_chassis_group[0].uuid) + + def test_update_lrp_with_ha_chassis_group_different_txn(self): + mac = next(net_utils.random_mac_generator(['ca', 'fe', 'ca', 'fe'])) + networks = ['192.0.2.0/24'] + lr_name = uuidutils.generate_uuid() + lrp_name = uuidutils.generate_uuid() + self.nbapi.lr_add(lr_name).execute(check_error=True) + self.nbapi.add_lrouter_port( + lrp_name, lr_name, mac=mac, + networks=networks).execute(check_error=True) + + # Create the HCG and update the LRP in two consecutive transactions. + hcg = self.nbapi.ha_chassis_group_with_hc_add( + uuidutils.generate_uuid(), {'ch1': 1, 'ch2': 2}).execute( + check_error=True) + self.nbapi.update_lrouter_port( + lrp_name, ha_chassis_group=hcg.uuid).execute(check_error=True) + + lrp = self.nbapi.lrp_get(lrp_name).execute(check_error=True) + self.assertEqual(hcg.uuid, lrp.ha_chassis_group[0].uuid) + class TestIgnoreConnectionTimeout(BaseOvnIdlTest): @classmethod From d93c35ff7ca9ae3a1829510ac9e5c3242631a087 Mon Sep 17 00:00:00 2001 From: Rodolfo Alonso Hernandez Date: Tue, 22 Apr 2025 16:42:03 +0000 Subject: [PATCH 6/8] Maintenance method to update the LRP from GC to HCG The new maintenance method ``migrate_lrp_gateway_chassis_to_ha_chassis_group``, replaces the ``Logical_Router_Port`` ``Gateway_Chassis`` with the equivalent ``HA_Chassis_Group``, that is the preferred method for HA scheduling in OVN, as reported in the LP bug. Partial-Bug: #2092271 Signed-off-by: Rodolfo Alonso Hernandez Change-Id: I8994415f97bca575b7104824ef889da0235c9a8b --- .../ovn/mech_driver/ovsdb/maintenance.py | 35 +++++++++++++++++ .../ovn/mech_driver/ovsdb/test_maintenance.py | 39 +++++++++++++++++++ 2 files changed, 74 insertions(+) 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 2783ec84102..036d5f536aa 100644 --- a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/maintenance.py +++ b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/maintenance.py @@ -1160,6 +1160,41 @@ def update_qos_fip_rule_priority(self): raise periodics.NeverAgain() + # TODO(ralonsoh): Remove this method in F+3 cycle (2nd next SLURP release) + @has_lock_periodic( + periodic_run_limit=ovn_const.MAINTENANCE_TASK_RETRY_LIMIT, + spacing=ovn_const.MAINTENANCE_ONE_RUN_TASK_SPACING, + run_immediately=True) + def migrate_lrp_gateway_chassis_to_ha_chassis_group(self): + """Migrate the LRP Gateway_Chassis to HA_Chassis_Group""" + with self._nb_idl.transaction(check_error=True) as txn: + for lrp in self._nb_idl.db_list_rows( + 'Logical_Router_Port').execute(check_error=True): + if not lrp.gateway_chassis: + continue + + chassis_prio = {} + for gc in lrp.gateway_chassis: + chassis_prio[gc.chassis_name] = gc.priority + r_name = lrp.external_ids.get( + ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY) + if not r_name: + LOG.warning(f'Logical_Router_Port {lrp.name} does not ' + 'have the router name in external_ids.') + continue + + # Add the new HA_Chassis_Group and assign to the LRP. + hcg_cmd = txn.add(self._nb_idl.ha_chassis_group_with_hc_add( + r_name, chassis_prio, may_exist=True)) + txn.add(self._nb_idl.db_set( + 'Logical_Router_Port', lrp.uuid, + ('ha_chassis_group', hcg_cmd))) + # Unset the Gateway_Chassis in the LRP. + txn.add(self._nb_idl.db_clear( + 'Logical_Router_Port', lrp.uuid, 'gateway_chassis')) + + raise periodics.NeverAgain() + class HashRingHealthCheckPeriodics: 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 5a7ea7a6158..f241494cc95 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 @@ -27,6 +27,7 @@ from neutron_lib import constants as n_const from neutron_lib import context as n_context from neutron_lib.exceptions import l3 as lib_l3_exc +from neutron_lib.utils import net as net_utils from oslo_utils import uuidutils from sqlalchemy.dialects.mysql import dialect as mysql_dialect @@ -1480,6 +1481,44 @@ def test_update_qos_fip_rule_priority(self): else: self.assertEqual(def_prio, qos_rule.priority) + def test_migrate_lrp_gateway_chassis_to_ha_chassis_group(self): + mac = next(net_utils.random_mac_generator(['ca', 'fe', 'ca', 'fe'])) + networks = ['192.0.2.0/24'] + lr_name = uuidutils.generate_uuid() + lrp_name = uuidutils.generate_uuid() + gateway_chassis = ['gw_ch1', 'gw_ch2', 'gw_ch3'] + + self.nb_api.lr_add(lr_name).execute(check_error=True) + ext_ids = {ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY: lr_name} + self.nb_api.add_lrouter_port( + lrp_name, lr_name, mac=mac, + networks=networks, gateway_chassis=gateway_chassis, + external_ids=ext_ids).execute(check_error=True) + + hcg = self.nb_api.lookup('HA_Chassis_Group', lr_name, default=None) + self.assertIsNone(hcg) + lr = self.nb_api.lookup('Logical_Router_Port', lrp_name) + chassis_prio = {} + for gc in lr.gateway_chassis: + chassis_prio[gc.chassis_name] = gc.priority + + self.assertRaises( + periodics.NeverAgain, + self.maint.migrate_lrp_gateway_chassis_to_ha_chassis_group) + + hcg = self.nb_api.lookup('HA_Chassis_Group', lr_name, default=None) + self.assertEqual(len(chassis_prio), len(hcg.ha_chassis)) + for ha_chassis in hcg.ha_chassis: + try: + # The priority and the chassis_name of the former + # Gateway_Chassis registers must match the new HA_Chassis ones. + self.assertEqual(ha_chassis.priority, + chassis_prio.pop(ha_chassis.chassis_name)) + except KeyError: + self.fail(f'HA_Chassis with chassis name ' + f'{ha_chassis.chassis_name} not present in the ' + f'chassis list') + class TestLogMaintenance(_TestMaintenanceHelper, test_log_driver.LogApiTestCaseBase): From 094c9f1a88a3924b5ffdfabab56c75a5eabaf5e3 Mon Sep 17 00:00:00 2001 From: Rodolfo Alonso Hernandez Date: Tue, 15 Apr 2025 20:56:25 +0000 Subject: [PATCH 7/8] Use HA_Chassis_Group in the OVN L3 scheduler TODO: * Testing NOTE: this method cannot be merged alone, it would need ``schedule_unhosted_gateways`` too and a migration strategy (a maintenance method) The NB API method ``schedule_new_gateway`` now creates a ``HA_Chassis_Group`` and its ``HA_Chassis`` registers, and associates it to the gateway ``Logical_Router_Port``. The usage og ``Gateway_Chassis`` is deprecated. Partial-Bug: #2092271 Change-Id: If38f405428a8b27bf551db48c4e96d75deeb09fc Signed-off-by: Rodolfo Alonso Hernandez --- neutron/common/ovn/utils.py | 10 +++++++ .../drivers/ovn/mech_driver/ovsdb/commands.py | 26 ++++++++++++------- 2 files changed, 27 insertions(+), 9 deletions(-) diff --git a/neutron/common/ovn/utils.py b/neutron/common/ovn/utils.py index 1432072205d..c7e294f67c1 100644 --- a/neutron/common/ovn/utils.py +++ b/neutron/common/ovn/utils.py @@ -903,6 +903,16 @@ def get_chassis_without_azs(chassis_list): get_chassis_availability_zones(ch)} +def get_chassis_priority(chassis_list): + """Given a chassis list, returns a dictionary with chassis name and prio + + The chassis list is ordered according to the priority: the first one is the + highest priority chassis, the last one is the least priority chassis. + """ + return {chassis: prio + 1 for prio, chassis + in enumerate(reversed(chassis_list))} + + def parse_ovn_lb_port_forwarding(ovn_rtr_lb_pfs): """Return a dictionary compatible with port forwarding from OVN lb.""" result = {} diff --git a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/commands.py b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/commands.py index f4bccdb6747..e97b3937575 100644 --- a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/commands.py +++ b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/commands.py @@ -432,9 +432,12 @@ def run_idl(self, txn): # the top. index = chassis.index(primary) chassis[0], chassis[index] = chassis[index], chassis[0] - setattr( - lrouter_port, - *_add_gateway_chassis(self.api, txn, self.g_name, chassis)) + chassis_priority = utils.get_chassis_priority(chassis) + lrouter_name = lrouter_port.external_ids[ + ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY] + hcg = _sync_ha_chassis_group(txn, self.api, lrouter_name, + chassis_priority, may_exist=True) + setattr(lrouter_port, 'ha_chassis_group', ovsdbapp_utils.get_uuid(hcg)) class ScheduleNewGatewayCommand(command.BaseCommand): @@ -459,8 +462,11 @@ def run_idl(self, txn): self.api, self.sb_api, self.g_name, candidates=candidates, target_lrouter=lrouter) if chassis: - setattr(lrouter_port, - *_add_gateway_chassis(self.api, txn, self.g_name, chassis)) + chassis_priority = utils.get_chassis_priority(chassis) + hcg = _sync_ha_chassis_group(txn, self.api, self.lrouter_name, + chassis_priority, may_exist=True) + setattr(lrouter_port, 'ha_chassis_group', + ovsdbapp_utils.get_uuid(hcg)) class LrDelCommand(ovn_nb_commands.LrDelCommand): @@ -1053,15 +1059,17 @@ def run_idl(self, txn): # Remove the router pinning to a chassis (if any). lrouter.delkey('options', 'chassis') - # Remove the HA_Chassis_Group of the router (if any). + for gw_port in self.api.get_lrouter_gw_ports(lrouter.name): + gw_port.ha_chassis_group = [] + lrouter.delvalue('ports', gw_port) + + # Remove the HA_Chassis_Group of the router (if any), after + # removing it from the gateway Logical_Router_Ports. hcg = self.api.lookup('HA_Chassis_Group', lrouter.name, default=None) if hcg: hcg.delete() - for gw_port in self.api.get_lrouter_gw_ports(lrouter.name): - lrouter.delvalue('ports', gw_port) - class SetLSwitchPortToVirtualTypeCommand(command.BaseCommand): def __init__(self, api, lport, vip, parent, if_exists): From 106d4f45c9d16183a3619b29356f442e1e7001dd Mon Sep 17 00:00:00 2001 From: Terry Wilson Date: Tue, 11 Jun 2024 11:37:11 -0500 Subject: [PATCH 8/8] Avoid race condition by using persist_uuid When creating a network and adding a port, the API requests will often be handled by different workers. Since each worker maintains its own internal copy of the OVN DBs, it is possible that the request for creating the port arrives at a worker that has not yet received notification that the Logical Switch for the network has been created. When the code for adding the Logical Switch Port is called, it tries to look up the LS, and fails. Currently, a retry-and-wait solution is in place for the LS/LSP issue in create_port(), but a new feature introduced in OVS 3.1 allows specifying the UUID of the LS, so that we can use a value known to the other worker, and add the LSP to the LS by that UUID even if the LS isn't in our local DB cache. This allows the ovsdb-server to be the source of truth on the existance of the LS. There are lots of other interactions that could result in this kind of race condition, any create/update of objects that happen rapidly on systems under high load for example. This method of creating and referencing objects should be generalizable. In this patch, the UUID for the LS is defined as the UUID for the neutron network. The name of the LS continues to be neutron-$UUID to match existing usage and to keep lookups by name working. Change-Id: Icc27c2b8825d7f96c9dac87dec8bbb55d493d942 --- neutron/common/ovn/utils.py | 2 + .../drivers/ovn/mech_driver/mech_driver.py | 4 ++ .../drivers/ovn/mech_driver/ovsdb/commands.py | 59 ++++++++++++++++--- .../ovn/mech_driver/ovsdb/impl_idl_ovn.py | 11 +++- .../ovn/mech_driver/ovsdb/ovn_client.py | 15 +++-- .../ovn/mech_driver/ovsdb/ovs_fixes.py | 37 ++++++++++++ .../ovn/mech_driver/test_mech_driver.py | 23 +++++--- requirements.txt | 2 +- 8 files changed, 129 insertions(+), 24 deletions(-) create mode 100644 neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovs_fixes.py diff --git a/neutron/common/ovn/utils.py b/neutron/common/ovn/utils.py index c7e294f67c1..5b2890dd7ac 100644 --- a/neutron/common/ovn/utils.py +++ b/neutron/common/ovn/utils.py @@ -68,6 +68,8 @@ 'HAChassisGroupInfo', ['group_name', 'chassis_list', 'az_hints', 'ignore_chassis', 'external_ids']) +_OVS_PERSIST_UUID = _SENTINEL = object() + class OvsdbClientCommand: _CONNECTION = 0 diff --git a/neutron/plugins/ml2/drivers/ovn/mech_driver/mech_driver.py b/neutron/plugins/ml2/drivers/ovn/mech_driver/mech_driver.py index ec87494a1bc..af1dfcff3ee 100644 --- a/neutron/plugins/ml2/drivers/ovn/mech_driver/mech_driver.py +++ b/neutron/plugins/ml2/drivers/ovn/mech_driver/mech_driver.py @@ -70,6 +70,7 @@ from neutron.plugins.ml2.drivers.ovn.mech_driver.ovsdb import maintenance from neutron.plugins.ml2.drivers.ovn.mech_driver.ovsdb import ovn_client from neutron.plugins.ml2.drivers.ovn.mech_driver.ovsdb import ovn_db_sync +from neutron.plugins.ml2.drivers.ovn.mech_driver.ovsdb import ovs_fixes from neutron.plugins.ml2.drivers.ovn.mech_driver.ovsdb import worker from neutron import service from neutron.services.logapi.drivers.ovn import driver as log_driver @@ -419,6 +420,9 @@ def post_fork_initialize(self, resource, event, trigger, payload=None): self._post_fork_event.clear() self._ovn_client_inst = None + # Patch python-ovs for fixes not yet released + ovs_fixes.apply_ovs_fixes() + if worker_class == wsgi.WorkerService: self._setup_hash_ring() diff --git a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/commands.py b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/commands.py index e97b3937575..b1ee143aa18 100644 --- a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/commands.py +++ b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/commands.py @@ -17,8 +17,10 @@ import uuid from oslo_utils import timeutils +from ovs.db import idl as ovs_idl from ovsdbapp.backend.ovs_idl import command from ovsdbapp.backend.ovs_idl import idlutils +from ovsdbapp.backend.ovs_idl import rowview from ovsdbapp.schema.ovn_northbound import commands as ovn_nb_commands from ovsdbapp import utils as ovsdbapp_utils @@ -155,21 +157,64 @@ def run_idl(self, txn): self.result = self.api.nb_global.nb_cfg +class AddNetworkCommand(command.AddCommand): + table_name = 'Logical_Switch' + + def __init__(self, api, network_id, may_exist=False, **columns): + super().__init__(api) + self.network_uuid = uuid.UUID(str(network_id)) + self.may_exist = may_exist + self.columns = columns + + def run_idl(self, txn): + table = self.api.tables[self.table_name] + try: + ls = table.rows[self.network_uuid] + if self.may_exist: + self.result = rowview.RowView(ls) + return + msg = _("Switch %s already exists") % self.network_uuid + raise RuntimeError(msg) + except KeyError: + # Adding a new LS + if utils.ovs_persist_uuid_supported(txn.idl): + ls = txn.insert(table, new_uuid=self.network_uuid, + persist_uuid=True) + else: + ls = txn.insert(table) + self.set_columns(ls, **self.columns) + ls.name = utils.ovn_name(self.network_uuid) + self.result = ls.uuid + + class AddLSwitchPortCommand(command.BaseCommand): - def __init__(self, api, lport, lswitch, may_exist, **columns): + def __init__(self, api, lport, lswitch, may_exist, network_id=None, + **columns): super().__init__(api) self.lport = lport self.lswitch = lswitch self.may_exist = may_exist + self.network_uuid = uuid.UUID(str(network_id)) if network_id else None self.columns = columns def run_idl(self, txn): try: + # We must look in the local cache first, because the LS may have + # been created as part of the current transaction. or in the case + # of adding an LSP to a LS that was created before persist_uuid lswitch = idlutils.row_by_value(self.api.idl, 'Logical_Switch', 'name', self.lswitch) except idlutils.RowNotFound: - msg = _("Logical Switch %s does not exist") % self.lswitch - raise RuntimeError(msg) + if self.network_uuid and utils.ovs_persist_uuid_supported(txn.idl): + # Create a "fake" row with the right UUID so python-ovs creates + # a transaction referencing the Row, even though we might not + # have received the update for the row ourselves. + lswitch = ovs_idl.Row(self.api.idl, + self.api.tables['Logical_Switch'], + uuid=self.network_uuid, data={}) + else: + msg = _("Logical Switch %s does not exist") % self.lswitch + raise RuntimeError(msg) if self.may_exist: port = idlutils.row_by_value(self.api.idl, 'Logical_Switch_Port', 'name', @@ -255,8 +300,8 @@ def run_idl(self, txn): else: new_port_dhcp_opts.add(dhcpv6_options.result) port.dhcpv6_options = [dhcpv6_options.result] - for uuid in cur_port_dhcp_opts - new_port_dhcp_opts: - self.api._tables['DHCP_Options'].rows[uuid].delete() + for uuid_ in cur_port_dhcp_opts - new_port_dhcp_opts: + self.api._tables['DHCP_Options'].rows[uuid_].delete() external_ids_update = self.external_ids_update or {} external_ids = getattr(port, 'external_ids', {}) @@ -338,8 +383,8 @@ def run_idl(self, txn): # Delete DHCP_Options records no longer referred by this port. cur_port_dhcp_opts = get_lsp_dhcp_options_uuids( lport, self.lport) - for uuid in cur_port_dhcp_opts: - self.api._tables['DHCP_Options'].rows[uuid].delete() + for uuid_ in cur_port_dhcp_opts: + self.api._tables['DHCP_Options'].rows[uuid_].delete() _delvalue_from_list(lswitch, 'ports', lport) self.api._tables['Logical_Switch_Port'].rows[lport.uuid].delete() diff --git a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/impl_idl_ovn.py b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/impl_idl_ovn.py index 3f8885f15bd..f21bf79db01 100644 --- a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/impl_idl_ovn.py +++ b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/impl_idl_ovn.py @@ -275,10 +275,17 @@ def transaction(self, *args, **kwargs): if revision_mismatch_raise: raise e + def ls_add(self, switch=None, may_exist=False, network_id=None, **columns): + if network_id is None: + return super().ls_add(switch, may_exist, **columns) + return cmd.AddNetworkCommand(self, network_id, may_exist=may_exist, + **columns) + def create_lswitch_port(self, lport_name, lswitch_name, may_exist=True, - **columns): + network_id=None, **columns): return cmd.AddLSwitchPortCommand(self, lport_name, lswitch_name, - may_exist, **columns) + may_exist, network_id=network_id, + **columns) def set_lswitch_port(self, lport_name, external_ids_update=None, if_exists=True, **columns): 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 0fe4b285115..1f74621e35e 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 @@ -590,9 +590,11 @@ def create_port(self, context, port): # controller does not yet see that network in its local cache of the # OVN northbound database. Check if the logical switch is present # or not in the idl's local copy of the database before creating - # the lswitch port. - self._nb_idl.check_for_row_by_value_and_retry( - 'Logical_Switch', 'name', lswitch_name) + # the lswitch port. Once we require an ovs version with working + # persist_uuid support, this can be removed. + if not utils.ovs_persist_uuid_supported(self._nb_idl): + self._nb_idl.check_for_row_by_value_and_retry( + 'Logical_Switch', 'name', lswitch_name) with self._nb_idl.transaction(check_error=True) as txn: dhcpv4_options, dhcpv6_options = self.update_port_dhcp_options( @@ -604,6 +606,7 @@ def create_port(self, context, port): kwargs = { 'lport_name': port['id'], 'lswitch_name': lswitch_name, + 'network_id': port['network_id'], 'addresses': port_info.addresses, 'external_ids': external_ids, 'parent_name': port_info.parent_name, @@ -2074,6 +2077,7 @@ def create_provnet_port(self, network_id, segment, txn=None, cmd = self._nb_idl.create_lswitch_port( lport_name=utils.ovn_provnet_port_name(segment['id']), lswitch_name=utils.ovn_name(network_id), + network_id=network_id, addresses=[ovn_const.UNKNOWN_ADDR], external_ids={}, type=ovn_const.LSP_TYPE_LOCALNET, @@ -2136,14 +2140,13 @@ def create_network(self, context, network): # UUID. This provides an easy way to refer to the logical switch # without having to track what UUID OVN assigned to it. lswitch_params = self._gen_network_parameters(network) - lswitch_name = utils.ovn_name(network['id']) # NOTE(mjozefcz): Remove this workaround when bug # 1869877 will be fixed. segments = segments_db.get_network_segments( context, network['id']) with self._nb_idl.transaction(check_error=True) as txn: - txn.add(self._nb_idl.ls_add(lswitch_name, **lswitch_params, - may_exist=True)) + txn.add(self._nb_idl.ls_add(network_id=network['id'], + **lswitch_params, may_exist=True)) for segment in segments: if segment.get(segment_def.PHYSICAL_NETWORK): self.create_provnet_port(network['id'], segment, txn=txn, diff --git a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovs_fixes.py b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovs_fixes.py new file mode 100644 index 00000000000..e8d2d81bd57 --- /dev/null +++ b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovs_fixes.py @@ -0,0 +1,37 @@ +# Copyright (c) 2024 +# 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 ovs.db import idl +import ovs.ovsuuid + + +# Temporarily fix ovs.db.idl.Transaction._substitute_uuids to support handling +# the persist_uuid feature +def _substitute_uuids(self, json): + if isinstance(json, (list, tuple)): + if (len(json) == 2 and + json[0] == 'uuid' and + ovs.ovsuuid.is_valid_string(json[1])): + uuid = ovs.ovsuuid.from_string(json[1]) + row = self._txn_rows.get(uuid, None) + if row and row._data is None and not row._persist_uuid: + return ["named-uuid", idl._uuid_name_from_uuid(uuid)] + else: + return [self._substitute_uuids(elem) for elem in json] + return json + + +def apply_ovs_fixes(): + idl.Transaction._substitute_uuids = _substitute_uuids diff --git a/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/test_mech_driver.py b/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/test_mech_driver.py index 7bd6e1bc386..5831b13bd1b 100644 --- a/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/test_mech_driver.py +++ b/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/test_mech_driver.py @@ -106,6 +106,8 @@ def setUp(self): neutron_agent.AgentCache().get_agents.return_value = [agent1] self.mock_vp_parents = mock.patch.object( ovn_utils, 'get_virtual_port_parents', return_value=None).start() + mock.patch.object(ovn_utils, 'ovs_persist_uuid_supported', + return_value=True).start() def _add_chassis_private(self, nb_cfg, name=None): chassis_private = mock.Mock() @@ -881,15 +883,19 @@ def test_network_precommit(self): self.mech_driver.update_network_precommit, fake_network_context) + def _verify_ls_add(self, net_id, may_exist=True, **kwargs): + ls_add = self.mech_driver._ovn_client._nb_idl.ls_add + ls_add.assert_called_once_with( + external_ids=mock.ANY, + may_exist=may_exist, network_id=net_id, **kwargs) + def _create_network_igmp_snoop(self, enabled): cfg.CONF.set_override('igmp_snooping_enable', enabled, group='OVS') - nb_idl = self.mech_driver._ovn_client._nb_idl net = self._make_network(self.fmt, name='net1', admin_state_up=True)['network'] value = 'true' if enabled else 'false' - nb_idl.ls_add.assert_called_once_with( - ovn_utils.ovn_name(net['id']), external_ids=mock.ANY, - may_exist=True, + self._verify_ls_add( + net_id=net['id'], other_config={ovn_const.MCAST_SNOOP: value, ovn_const.MCAST_FLOOD_UNREGISTERED: 'false', ovn_const.VLAN_PASSTHRU: 'false'}) @@ -901,7 +907,6 @@ def test_create_network_igmp_snoop_disabled(self): self._create_network_igmp_snoop(enabled=False) def _create_network_vlan_passthru(self, vlan_transparent, qinq): - nb_idl = self.mech_driver._ovn_client._nb_idl net = self._make_network( self.fmt, name='net1', as_admin=True, @@ -917,9 +922,8 @@ def _create_network_vlan_passthru(self, vlan_transparent, qinq): 'provider:physical_network': 'physnet1'})['network'] value = 'true' if vlan_transparent or qinq else 'false' expected_fdb_age_treshold = ovn_conf.get_fdb_age_threshold() - nb_idl.ls_add.assert_called_once_with( - ovn_utils.ovn_name(net['id']), external_ids=mock.ANY, - may_exist=True, + self._verify_ls_add( + net_id=net['id'], other_config={ ovn_const.MCAST_SNOOP: 'false', ovn_const.MCAST_FLOOD_UNREGISTERED: 'false', @@ -958,6 +962,7 @@ def test_create_network_create_localnet_port_physical_network_type(self): self.context, net['id']) nb_idl.create_lswitch_port.assert_called_once_with( addresses=[ovn_const.UNKNOWN_ADDR], + network_id=net['id'], external_ids={}, lport_name=ovn_utils.ovn_provnet_port_name(segments[0]['id']), lswitch_name=ovn_utils.ovn_name(net['id']), @@ -3477,6 +3482,7 @@ def test_create_segment_create_localnet_port(self): segmentation_id=200, network_type='vlan')['segment'] ovn_nb_api.create_lswitch_port.assert_called_once_with( addresses=[ovn_const.UNKNOWN_ADDR], + network_id=net['id'], external_ids={}, lport_name=ovn_utils.ovn_provnet_port_name(new_segment['id']), lswitch_name=ovn_utils.ovn_name(net['id']), @@ -3495,6 +3501,7 @@ def test_create_segment_create_localnet_port(self): segmentation_id=300, network_type='vlan')['segment'] ovn_nb_api.create_lswitch_port.assert_called_once_with( addresses=[ovn_const.UNKNOWN_ADDR], + network_id=net['id'], external_ids={}, lport_name=ovn_utils.ovn_provnet_port_name(new_segment['id']), lswitch_name=ovn_utils.ovn_name(net['id']), diff --git a/requirements.txt b/requirements.txt index 161c7b69832..afd39c66ae8 100644 --- a/requirements.txt +++ b/requirements.txt @@ -44,7 +44,7 @@ oslo.versionedobjects>=1.35.1 # Apache-2.0 osprofiler>=2.3.0 # Apache-2.0 os-ken>=3.0.0 # Apache-2.0 os-resource-classes>=1.1.0 # Apache-2.0 -ovs>=2.12.0 # Apache-2.0 +ovs>3.3.0 # Apache-2.0 ovsdbapp>=2.11.0 # Apache-2.0 psutil>=6.1.0 # BSD pyroute2>=0.7.3;sys_platform!='win32' # Apache-2.0 (+ dual licensed GPL2)