From 0bae4b70b6fc53cf5502622677a95ee50cc319a1 Mon Sep 17 00:00:00 2001 From: Frode Nordahl Date: Wed, 21 Jun 2023 08:59:24 +0200 Subject: [PATCH] [ovn] Make scheduling of unhosted gateways aware of current transaction At present, whenever multiple additions/updates are made to LRPs with gateway_chassis, each update is put in separate transactions in an attempt to ensure the scheduler operates on updated information for each iteration. This is problematic because we don't have the luxury of creating separate transactions for updates in all parts of the code base, and it is also not very efficient. The OVSDBapp library wraps the OVS Python IDL and provides different semantics. Most notably the OVSDBapp represents a Transaction as a list of command objects with `run_idl` methods for execution at some point in the future. The main loop and the command objects are not aware of changes made in the current transaction until it is committed. Fortunately, as an ovsdbapp transaction is committed, the underlying OVS Python IDL is kept up to date during the course of the transaction [0][1][2]. Move implementation of scheduling of unhosted gateways into an ovsdbapp command, using a plugin reference to the Neutron OVNClient class for any calls into the Neutron code, allowing scheduling decisions to be made on up to date data as the transaction is applied. 0: https://github.com/openvswitch/ovs/blob/e3ba0be48ca4/python/ovs/db/idl.py#L1316 1: https://github.com/openvswitch/ovs/blob/e3ba0be48ca4/python/ovs/db/idl.py#L1400 2: https://github.com/openvswitch/ovs/blob/e3ba0be48ca4/python/ovs/db/idl.py#L2083 Partial-Bug: #2002687 Co-Authored-By: Terry Wilson Co-Authored-By: Brian Haley Signed-off-by: Frode Nordahl Change-Id: I83bcf7fe838c0d6b0617c43439643e8443b2bdae --- .../ml2/drivers/ovn/mech_driver/ovsdb/api.py | 22 +++ .../drivers/ovn/mech_driver/ovsdb/commands.py | 71 ++++++++++ .../ovn/mech_driver/ovsdb/impl_idl_ovn.py | 7 + neutron/services/ovn_l3/plugin.py | 67 ++-------- .../functional/services/ovn_l3/test_plugin.py | 125 +++++++++++++----- neutron/tests/unit/fake_resources.py | 3 + .../ovn/mech_driver/ovsdb/test_commands.py | 61 +++++++++ .../tests/unit/services/ovn_l3/test_plugin.py | 111 +++++----------- 8 files changed, 302 insertions(+), 165 deletions(-) diff --git a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/api.py b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/api.py index a9ed49897d0..dffc9a1456d 100644 --- a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/api.py +++ b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/api.py @@ -125,6 +125,28 @@ class API(api.API, metaclass=abc.ABCMeta): :returns: :class:`Command` with no result """ + @abc.abstractmethod + def schedule_unhosted_gateways(self, g_name, sb_api, plugin, port_physnets, + all_gw_chassis, chassis_with_physnets, + chassis_with_azs): + """Schedule unhosted gateways + + :param g_name: The unique name of the lrouter port + :type g_name: string + :param sb_api: IDL for the OVN SB API + :type class: :class:`OvsdbSbOvnIdl` + :param plugin: The L3 plugin to call back to for lookups + :type plugin: :class:`OVNL3RouterPlugin` + :param port_physnets: Map of lrouter ports and their physnets + :type port_physnets: Dict[string,string] + :param all_gw_chassis: List of gateway chassis + :type all_gw_chassis: List[Option[string,:class:`RowView`]] + :param chassis_with_physnets: Map of chassis and their physnets + :type chassis_with_physnets: Dict[string,Set[string]] + :param chassis_with_azs: Map of chassis and their AZs + :type chassis_with_azs: Dict[string,Set[string]] + """ + @abc.abstractmethod def delete_lrouter_port(self, name, lrouter, if_exists=True): """Create a command to delete an OVN lrouter port 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 3b633822905..1ef15201ba7 100644 --- a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/commands.py +++ b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/commands.py @@ -25,6 +25,7 @@ from neutron.services.portforwarding.constants import PORT_FORWARDING_PREFIX from oslo_log import log + LOG = log.getLogger(__name__) RESOURCE_TYPE_MAP = { @@ -320,6 +321,76 @@ class UpdateLRouterCommand(command.BaseCommand): return +class ScheduleUnhostedGatewaysCommand(command.BaseCommand): + def __init__(self, nb_api, g_name, sb_api, plugin, port_physnets, + all_gw_chassis, chassis_with_physnets, chassis_with_azs): + super().__init__(api=nb_api) + self.g_name = g_name + self.sb_api = sb_api + self.scheduler = plugin.scheduler + self.ovn_client = plugin._ovn_client + self.port_physnets = port_physnets + self.all_gw_chassis = all_gw_chassis + self.chassis_with_physnets = chassis_with_physnets + self.chassis_with_azs = chassis_with_azs + + def run_idl(self, txn): + lrouter_port = self.api.lookup("Logical_Router_Port", self.g_name) + physnet = self.port_physnets.get( + self.g_name[len(ovn_const.LRP_PREFIX):]) + # Remove any invalid gateway chassis from the list, otherwise + # we can have a situation where all existing_chassis are invalid + existing_chassis = self.api.get_gateway_chassis_binding(self.g_name) + primary = existing_chassis[0] if existing_chassis else None + 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, + chassis_physnets=self.chassis_with_physnets, + existing_chassis=existing_chassis, az_hints=az_hints, + chassis_with_azs=self.chassis_with_azs)) + if existing_chassis != filtered_existing_chassis: + first_diff = None + for i in range(len(filtered_existing_chassis)): + if existing_chassis[i] != filtered_existing_chassis[i]: + first_diff = i + break + if first_diff is not None: + LOG.debug( + "A chassis for this gateway has been filtered. " + "Rebalancing priorities %s and lower", first_diff) + filtered_existing_chassis = filtered_existing_chassis[ + :max(first_diff, 1)] + + candidates = self.ovn_client.get_candidates_for_scheduling( + physnet, cms=self.all_gw_chassis, + chassis_physnets=self.chassis_with_physnets, + availability_zone_hints=az_hints) + chassis = self.scheduler.select( + self.api, self.sb_api, self.g_name, candidates=candidates, + existing_chassis=filtered_existing_chassis) + if primary and primary != chassis[0]: + if primary not in chassis: + LOG.debug("Primary gateway chassis %(old)s " + "has been removed from the system. Moving " + "gateway %(gw)s to other chassis %(new)s.", + {'gw': self.g_name, + 'old': primary, + 'new': chassis[0]}) + else: + LOG.debug("Gateway %s is hosted at %s.", self.g_name, primary) + # NOTE(mjozefcz): It means scheduler moved primary chassis + # to other gw based on scheduling method. But we don't + # want network flap - so moving actual primary to be on + # 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)) + + class AddLRouterPortCommand(command.BaseCommand): def __init__(self, api, name, lrouter, may_exist, **columns): super(AddLRouterPortCommand, self).__init__(api) 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 4103a87c4a9..d7af3c57677 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 @@ -431,6 +431,13 @@ class OvsdbNbOvnIdl(nb_impl_idl.OvnNbApiIdlImpl, Backend): return cmd.AddLRouterPortCommand(self, name, lrouter, may_exist, **columns) + def schedule_unhosted_gateways(self, g_name, sb_api, plugin, port_physnets, + all_gw_chassis, chassis_with_physnets, + chassis_with_azs): + return cmd.ScheduleUnhostedGatewaysCommand( + self, g_name, sb_api, plugin, port_physnets, all_gw_chassis, + chassis_with_physnets, chassis_with_azs) + def update_lrouter_port(self, name, if_exists=True, **columns): return cmd.UpdateLRouterPortCommand(self, name, if_exists, **columns) diff --git a/neutron/services/ovn_l3/plugin.py b/neutron/services/ovn_l3/plugin.py index 6067bcdefc4..dafdaf9b66c 100644 --- a/neutron/services/ovn_l3/plugin.py +++ b/neutron/services/ovn_l3/plugin.py @@ -296,61 +296,18 @@ class OVNL3RouterPlugin(service_base.ServicePluginBase, all_gw_chassis = self._sb_ovn.get_gateway_chassis_from_cms_options() chassis_with_azs = self._sb_ovn.get_chassis_and_azs() - for g_name in lrps: - physnet = port_physnet_dict.get(g_name[len(ovn_const.LRP_PREFIX):]) - # Remove any invalid gateway chassis from the list, otherwise - # we can have a situation where all existing_chassis are invalid - existing_chassis = self._nb_ovn.get_gateway_chassis_binding(g_name) - primary = existing_chassis[0] if existing_chassis else None - az_hints = self._nb_ovn.get_gateway_chassis_az_hints(g_name) - filtered_existing_chassis = \ - self.scheduler.filter_existing_chassis( - nb_idl=self._nb_ovn, gw_chassis=all_gw_chassis, - physnet=physnet, - chassis_physnets=chassis_with_physnets, - existing_chassis=existing_chassis, az_hints=az_hints, - chassis_with_azs=chassis_with_azs) - if existing_chassis != filtered_existing_chassis: - first_diff = None - for i in range(len(filtered_existing_chassis)): - if existing_chassis[i] != filtered_existing_chassis[i]: - first_diff = i - break - if first_diff is not None: - LOG.debug( - "A chassis for this gateway has been filtered. " - "Rebalancing priorities %s and lower", first_diff) - filtered_existing_chassis = filtered_existing_chassis[ - :max(first_diff, 1)] - - candidates = self._ovn_client.get_candidates_for_scheduling( - physnet, cms=all_gw_chassis, - chassis_physnets=chassis_with_physnets, - availability_zone_hints=az_hints) - chassis = self.scheduler.select( - self._nb_ovn, self._sb_ovn, g_name, candidates=candidates, - existing_chassis=filtered_existing_chassis) - if primary and primary != chassis[0]: - if primary not in chassis: - LOG.debug("Primary gateway chassis %(old)s " - "has been removed from the system. Moving " - "gateway %(gw)s to other chassis %(new)s.", - {'gw': g_name, - 'old': primary, - 'new': chassis[0]}) - else: - LOG.debug("Gateway %s is hosted at %s.", g_name, primary) - # NOTE(mjozefcz): It means scheduler moved primary chassis - # to other gw based on scheduling method. But we don't - # want network flap - so moving actual primary to be on - # the top. - index = chassis.index(primary) - chassis[0], chassis[index] = chassis[index], chassis[0] - # NOTE(dalvarez): Let's commit the changes in separate transactions - # as we will rely on those for scheduling subsequent gateways. - with self._nb_ovn.transaction(check_error=True) as txn: - txn.add(self._nb_ovn.update_lrouter_port( - g_name, gateway_chassis=chassis)) + with self._nb_ovn.transaction(check_error=True) as txn: + for g_name in lrps: + # NOTE(fnordahl): Make scheduling decissions in ovsdbapp + # command so that scheduling is done based on up to date + # information as the transaction is applied. + # + # We pass in a reference to our class instance so that the + # ovsdbapp command can call the scheduler methods from within + # its context. + txn.add(self._nb_ovn.schedule_unhosted_gateways( + g_name, self._sb_ovn, self, port_physnet_dict, + all_gw_chassis, chassis_with_physnets, chassis_with_azs)) @staticmethod @registry.receives(resources.SUBNET, [events.AFTER_UPDATE]) diff --git a/neutron/tests/functional/services/ovn_l3/test_plugin.py b/neutron/tests/functional/services/ovn_l3/test_plugin.py index 984ed6ae2d8..a3679629c5a 100644 --- a/neutron/tests/functional/services/ovn_l3/test_plugin.py +++ b/neutron/tests/functional/services/ovn_l3/test_plugin.py @@ -94,6 +94,50 @@ class TestRouter(base.TestOVNFunctionalBase): lrp.name, gateway_chassis=[ovn_const.OVN_GATEWAY_INVALID_CHASSIS])) + def _get_gwc_dict(self): + sched_info = {} + for row in self.nb_api.db_list_rows("Logical_Router_Port").execute( + check_error=True): + for gwc in row.gateway_chassis: + chassis = sched_info.setdefault(gwc.chassis_name, {}) + chassis[gwc.priority] = chassis.get(gwc.priority, 0) + 1 + return sched_info + + def _create_routers_wait_pb(self, begin, n, gw_info=None, + bind_chassis=None): + routers = [] + for i in range(begin, n + begin): + try: + router = self._create_router('router%d' % i, gw_info=gw_info) + routers.append(router) + except db_exc.DBReferenceError: + # NOTE(ralonsoh): this is a workaround for LP#1956344. There + # seems to be a bug in SQLite3. The "port" DB object is not + # persistently stored in the DB and raises a "DBReferenceError" + # exception occasionally. + continue + + gw_port_id = router.get('gw_port_id') + logical_port = 'cr-lrp-%s' % gw_port_id + self.assertTrue(self.cr_lrp_pb_event.wait(logical_port), + msg='lrp %s failed to bind' % logical_port) + if bind_chassis: + self.sb_api.lsp_bind(logical_port, bind_chassis, + may_exist=True).execute(check_error=True) + return routers + + def _add_chassis(self, begin, n, physical_nets): + chassis_added = [] + for i in range(begin, begin + n): + chassis_added.append( + self.add_fake_chassis( + 'ovs-host%s' % i, + name=f'chassis-{i:02d}', + physical_nets=physical_nets, + other_config={ + 'ovn-cms-options': 'enable-chassis-as-gw'})) + return chassis_added + def test_gateway_chassis_on_router_gateway_port(self): ext2 = self._create_ext_network( 'ext2', 'flat', 'physnet3', None, "20.0.0.1", "20.0.0.0/24") @@ -569,15 +613,6 @@ class TestRouter(base.TestOVNFunctionalBase): len(gws['router']['external_gateways'])) def test_gateway_chassis_rebalance(self): - def _get_result_dict(): - sched_info = {} - for row in self.nb_api.tables[ - 'Logical_Router_Port'].rows.values(): - for gwc in row.gateway_chassis: - chassis = sched_info.setdefault(gwc.chassis_name, {}) - chassis[gwc.priority] = chassis.get(gwc.priority, 0) + 1 - return sched_info - ovn_client = self.l3_plugin._ovn_client chassis4 = self.add_fake_chassis( 'ovs-host4', physical_nets=['physnet4'], other_config={ @@ -588,27 +623,11 @@ class TestRouter(base.TestOVNFunctionalBase): gw_info = {'network_id': ext1['network']['id']} # Tries to create 5 routers with a gateway. Since we're using # physnet4, the chassis candidates will be chassis4 initially. - num_routers = 5 - for i in range(num_routers): - try: - router = self._create_router('router%d' % i, gw_info=gw_info) - except db_exc.DBReferenceError: - # NOTE(ralonsoh): this is a workaround for LP#1956344. There - # seems to be a bug in SQLite3. The "port" DB object is not - # persistently stored in the DB and raises a "DBReferenceError" - # exception occasionally. - num_routers -= 1 - continue - - gw_port_id = router.get('gw_port_id') - logical_port = 'cr-lrp-%s' % gw_port_id - self.assertTrue(self.cr_lrp_pb_event.wait(logical_port), - msg='lrp %s failed to bind' % logical_port) - self.sb_api.lsp_bind(logical_port, chassis4, - may_exist=True).execute(check_error=True) + num_routers = len(self._create_routers_wait_pb( + 1, 20, gw_info=gw_info, bind_chassis=chassis4)) self.l3_plugin.schedule_unhosted_gateways() expected = {chassis4: {1: num_routers}} - self.assertEqual(expected, _get_result_dict()) + self.assertEqual(expected, self._get_gwc_dict()) # Add another chassis as a gateway chassis chassis5 = self.add_fake_chassis( @@ -622,9 +641,9 @@ class TestRouter(base.TestOVNFunctionalBase): # Chassis4 should have all ports at Priority 2 self.l3_plugin.schedule_unhosted_gateways() - self.assertEqual({2: num_routers}, _get_result_dict()[chassis4]) + self.assertEqual({2: num_routers}, self._get_gwc_dict()[chassis4]) # Chassis5 should have all ports at Priority 1 - self.assertEqual({1: num_routers}, _get_result_dict()[chassis5]) + self.assertEqual({1: num_routers}, self._get_gwc_dict()[chassis5]) # delete chassis that hosts all the gateways self.del_fake_chassis(chassis4) @@ -633,7 +652,7 @@ class TestRouter(base.TestOVNFunctionalBase): # As Chassis4 has been removed so all gateways that were # hosted there are now primaries on chassis5 and have # priority 1. - self.assertEqual({1: num_routers}, _get_result_dict()[chassis5]) + self.assertEqual({1: num_routers}, self._get_gwc_dict()[chassis5]) def test_gateway_chassis_rebalance_max_chassis(self): chassis_list = [] @@ -669,3 +688,47 @@ class TestRouter(base.TestOVNFunctionalBase): "Logical_Router", lr_name, "options").execute(check_error=True) self.assertEqual(ovn_conf.get_ovn_mac_binding_age_threshold(), options[ovn_const.LR_OPTIONS_MAC_AGE_LIMIT]) + + def test_schedule_unhosted_gateways_single_transaction(self): + ext1 = self._create_ext_network( + 'ext1', 'flat', 'physnet6', None, "10.0.60.1", "10.0.60.0/24") + gw_info = {'network_id': ext1['network']['id']} + + # Attempt to add 4 routers, since there are no chassis all will be + # scheduled on the ovn_const.OVN_GATEWAY_INVALID_CHASSIS. + num_routers = len(self._create_routers_wait_pb(1, 4, gw_info)) + self.assertEqual( + {ovn_const.OVN_GATEWAY_INVALID_CHASSIS: {1: num_routers}}, + self._get_gwc_dict()) + + # Add 2 chassis and rebalance gateways. + # + # The ovsdb_monitor.ChassisEvent handler will attempt to schedule + # unhosted gateways as chassis are added. + # + # Temporarily mock it out while adding the chassis so that we can get a + # predictable result for the purpose of this test. + chassis_list = [] + with mock.patch.object( + self.l3_plugin, 'schedule_unhosted_gateways'): + chassis_list.extend(self._add_chassis(1, 2, ['physnet6'])) + self.assertEqual( + {ovn_const.OVN_GATEWAY_INVALID_CHASSIS: {1: num_routers}}, + self._get_gwc_dict()) + + # Wrap `self.l3_plugin._nb_ovn.transaction` so that we can assert on + # number of calls. + with mock.patch.object( + self.l3_plugin._nb_ovn, 'transaction', + wraps=self.l3_plugin._nb_ovn.transaction) as wrap_txn: + self.l3_plugin.schedule_unhosted_gateways() + # The server is alive and we can't control the exact number of + # calls made to `_nb_ovn.transaction`. We can however check that + # the number of calls is less than number of unhosted gateways. + self.assertLess(len(wrap_txn.mock_calls), num_routers) + + # Ensure the added gateways are spread evenly on the added chassis. + self.assertEqual( + {'chassis-01': {1: 2, 2: 2}, + 'chassis-02': {1: 2, 2: 2}}, + self._get_gwc_dict()) diff --git a/neutron/tests/unit/fake_resources.py b/neutron/tests/unit/fake_resources.py index 23628aba0f0..a6d32ced419 100644 --- a/neutron/tests/unit/fake_resources.py +++ b/neutron/tests/unit/fake_resources.py @@ -40,6 +40,7 @@ class FakeOvsdbNbOvnIdl(object): self.dhcp_options_table = FakeOvsdbTable.create_one_ovsdb_table() self.nat_table = FakeOvsdbTable.create_one_ovsdb_table() self.port_group_table = FakeOvsdbTable.create_one_ovsdb_table() + self.gateway_chassis_table = FakeOvsdbTable.create_one_ovsdb_table() self._tables = {} self._tables['Logical_Switch'] = self.lswitch_table self._tables['Logical_Switch_Port'] = self.lsp_table @@ -52,6 +53,7 @@ class FakeOvsdbNbOvnIdl(object): self._tables['DHCP_Options'] = self.dhcp_options_table self._tables['NAT'] = self.nat_table self._tables['Port_Group'] = self.port_group_table + self._tables['Gateway_Chassis'] = self.gateway_chassis_table self.transaction = mock.MagicMock() self.create_transaction = mock.MagicMock() self.ls_add = mock.Mock() @@ -163,6 +165,7 @@ class FakeOvsdbNbOvnIdl(object): self.lrp_get = mock.Mock() self.get_schema_version = mock.Mock(return_value='3.6.0') self.get_lrouter_port = mock.Mock() + self.schedule_unhosted_gateways = mock.Mock() class FakeOvsdbSbOvnIdl(object): diff --git a/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_commands.py b/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_commands.py index f967f196738..122b73d3fc4 100644 --- a/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_commands.py +++ b/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_commands.py @@ -1220,3 +1220,64 @@ class TestDeleteLRouterExtGwCommand(TestBaseCommand): def test_delete_no_lrouter_exist_fail(self): self._test_delete_lrouter_no_lrouter_exist(if_exists=False) + + +class TestScheduleUnhostedGatewaysCommand(TestBaseCommand): + + @staticmethod + def _insert_gwc(table): + fake_gwc = fakes.FakeOvsdbRow.create_one_ovsdb_row() + table.rows[fake_gwc.uuid] = fake_gwc + return fake_gwc + + def test_schedule_unhosted_gateways_rebalances_lower_prios(self): + unhosted_gws = ['lrp-foo-1', 'lrp-foo-2', 'lrp-foo-3'] + port_physnets = {k[len(ovn_const.LRP_PREFIX):]: 'physnet1' + for k in unhosted_gws} + # we skip chasiss2 here since we assume it has been removed + chassis_mappings = { + 'chassis1': ['physnet1'], + 'chassis3': ['physnet1'], + 'chassis4': ['physnet1'], + } + chassis = ['chassis1', 'chassis3', 'chassis4'] + sb_api = mock.MagicMock() + plugin = mock.MagicMock() + plugin.scheduler.select.side_effect = [ + ['chassis1', 'chassis4', 'chassis3'], + ['chassis4', 'chassis3', 'chassis1'], + ['chassis4', 'chassis3', 'chassis1'], + ] + self.transaction.insert.side_effect = self._insert_gwc + + expected_mapping = { + 'lrp-foo-1': ['chassis1', 'chassis4', 'chassis3'], + 'lrp-foo-2': ['chassis4', 'chassis3', 'chassis1'], + 'lrp-foo-3': ['chassis4', 'chassis3', 'chassis1'], + } + + with mock.patch.object( + self.ovn_api, + 'get_gateway_chassis_binding', + side_effect=[ + ['chassis1', 'chassis2', 'chassis3', 'chassis4'], + ['chassis2', 'chassis4', 'chassis3', 'chassis1'], + ['chassis4', 'chassis3', 'chassis1', 'chassis2'], + ]): + for g_name in unhosted_gws: + lrouter_port = mock.MagicMock() + with mock.patch.object(self.ovn_api, 'lookup', + return_value=lrouter_port): + with mock.patch.object(idlutils, 'row_by_value', + side_effect=idlutils.RowNotFound): + cmd = commands.ScheduleUnhostedGatewaysCommand( + self.ovn_api, g_name, sb_api, plugin, + port_physnets, chassis, chassis_mappings, []) + cmd.run_idl(self.transaction) + self.assertEqual( + expected_mapping[g_name], + [ + self.ovn_api._tables[ + 'Gateway_Chassis'].rows[uuid].chassis_name + for uuid in lrouter_port.gateway_chassis + ]) diff --git a/neutron/tests/unit/services/ovn_l3/test_plugin.py b/neutron/tests/unit/services/ovn_l3/test_plugin.py index cdde85f8528..6fac450232f 100644 --- a/neutron/tests/unit/services/ovn_l3/test_plugin.py +++ b/neutron/tests/unit/services/ovn_l3/test_plugin.py @@ -1809,25 +1809,41 @@ class TestOVNL3RouterPlugin(test_mech_driver.Ml2PluginV2TestCase): self.l3_inst.schedule_unhosted_gateways() - self.mock_candidates.assert_has_calls([ - mock.call(mock.ANY, - chassis_physnets=chassis_mappings, - cms=chassis, availability_zone_hints=[])] * 3) - self.mock_schedule.assert_has_calls([ - mock.call(self.nb_idl(), self.sb_idl(), - 'lrp-foo-1', [], ['chassis1', 'chassis2']), - mock.call(self.nb_idl(), self.sb_idl(), - 'lrp-foo-2', [], ['chassis2']), - mock.call(self.nb_idl(), self.sb_idl(), - 'lrp-foo-3', [], [])]) - # make sure that for second port primary chassis stays untouched - self.nb_idl().update_lrouter_port.assert_has_calls([ + self.nb_idl().schedule_unhosted_gateways.assert_has_calls([ mock.call('lrp-foo-1', - gateway_chassis=['chassis1', 'chassis2', 'chassis3']), + mock.ANY, + mock.ANY, + {'foo-1': 'physnet1', + 'foo-2': 'physnet1', + 'foo-3': 'physnet1'}, + ['chassis1', 'chassis2', 'chassis3'], + {'chassis1': ['physnet1'], + 'chassis2': ['physnet1'], + 'chassis3': ['physnet1']}, + {}), mock.call('lrp-foo-2', - gateway_chassis=['chassis2', 'chassis1', 'chassis3']), + mock.ANY, + mock.ANY, + {'foo-1': 'physnet1', + 'foo-2': 'physnet1', + 'foo-3': 'physnet1'}, + ['chassis1', 'chassis2', 'chassis3'], + {'chassis1': ['physnet1'], + 'chassis2': ['physnet1'], + 'chassis3': ['physnet1']}, + {}), mock.call('lrp-foo-3', - gateway_chassis=['chassis3', 'chassis2', 'chassis1'])]) + mock.ANY, + mock.ANY, + {'foo-1': 'physnet1', + 'foo-2': 'physnet1', + 'foo-3': 'physnet1'}, + ['chassis1', 'chassis2', 'chassis3'], + {'chassis1': ['physnet1'], + 'chassis2': ['physnet1'], + 'chassis3': ['physnet1']}, + {}), + ]) @mock.patch('neutron.services.ovn_l3.plugin.OVNL3RouterPlugin.' '_get_gateway_port_physnet_mapping') @@ -1857,69 +1873,6 @@ class TestOVNL3RouterPlugin(test_mech_driver.Ml2PluginV2TestCase): self.nb_idl().get_unhosted_gateways.assert_called_once_with( {'foo-1': 'physnet1'}, mock.ANY, mock.ANY, mock.ANY) - @mock.patch('neutron.plugins.ml2.drivers.ovn.mech_driver.mech_driver.' - 'OVNMechanismDriver.list_availability_zones', lambda *_: []) - @mock.patch('neutron.services.ovn_l3.plugin.OVNL3RouterPlugin.' - '_get_gateway_port_physnet_mapping') - def test_schedule_unhosted_gateways_rebalances_lower_prios(self, get_gppm): - unhosted_gws = ['lrp-foo-1', 'lrp-foo-2', 'lrp-foo-3'] - get_gppm.return_value = {k[len(ovn_const.LRP_PREFIX):]: 'physnet1' - for k in unhosted_gws} - # we skip chasiss2 here since we assume it has been removed - chassis_mappings = { - 'chassis1': ['physnet1'], - 'chassis3': ['physnet1'], - 'chassis4': ['physnet1'], - } - chassis = ['chassis1', 'chassis3', 'chassis4'] - self.sb_idl().get_chassis_and_physnets.return_value = ( - chassis_mappings) - self.sb_idl().get_gateway_chassis_from_cms_options.return_value = ( - chassis) - self.nb_idl().get_unhosted_gateways.return_value = unhosted_gws - self.mock_candidates.return_value = chassis - # all ports have 4 chassis (including chassis2 that will be removed) - # the ports are not perfectly balanced (but this is realistic with a) - # few router creations and deletions - existing_port_bindings = [ - ['chassis1', 'chassis2', 'chassis3', 'chassis4'], - ['chassis2', 'chassis4', 'chassis3', 'chassis1'], - ['chassis4', 'chassis3', 'chassis1', 'chassis2']] - self.nb_idl().get_gateway_chassis_binding.side_effect = ( - existing_port_bindings) - # for 1. port reschedule all besides the first - # for 2. port reschedule all besides the new first (chassis 4) - # for 3. port keep all and drop the last - self.mock_schedule.side_effect = [ - ['chassis1', 'chassis4', 'chassis3'], - ['chassis4', 'chassis3', 'chassis1'], - ['chassis4', 'chassis3', 'chassis1']] - - self.l3_inst.schedule_unhosted_gateways() - - self.mock_candidates.assert_has_calls([ - mock.call(mock.ANY, - chassis_physnets=chassis_mappings, - cms=chassis, availability_zone_hints=[])] * 3) - self.mock_schedule.assert_has_calls([ - mock.call(self.nb_idl(), self.sb_idl(), 'lrp-foo-1', - ['chassis1', 'chassis3', 'chassis4'], - ['chassis1']), - mock.call(self.nb_idl(), self.sb_idl(), 'lrp-foo-2', - ['chassis1', 'chassis3', 'chassis4'], - ['chassis4']), - mock.call(self.nb_idl(), self.sb_idl(), 'lrp-foo-3', - ['chassis1', 'chassis3', 'chassis4'], - ['chassis4', 'chassis3', 'chassis1'])]) - # make sure that the primary chassis stays untouched - self.nb_idl().update_lrouter_port.assert_has_calls([ - mock.call('lrp-foo-1', - gateway_chassis=['chassis1', 'chassis4', 'chassis3']), - mock.call('lrp-foo-2', - gateway_chassis=['chassis4', 'chassis3', 'chassis1']), - mock.call('lrp-foo-3', - gateway_chassis=['chassis4', 'chassis3', 'chassis1'])]) - @mock.patch('neutron.plugins.ml2.plugin.Ml2Plugin.get_network') @mock.patch('neutron.plugins.ml2.plugin.Ml2Plugin.get_networks') @mock.patch('neutron.plugins.ml2.drivers.ovn.mech_driver.ovsdb.'