From d67d1c273668ad4bb0d6906b7684f4a7b095c8d4 Mon Sep 17 00:00:00 2001 From: Frode Nordahl Date: Fri, 17 Mar 2023 18:42:53 +0100 Subject: [PATCH] [ovn] Drop use of OVN_GW_PORT_EXT_ID_KEY At present Neutron maintains an external_id on the Logical_Router (LR) representing the gw port (singluar). This is problematic when introducing multiple gateway ports. Instead we can find Logical Router Port (LRP) that act as gateways for the LR at runtime by looking for configuration present on all GW ports. Partial-Bug: #2002687 Signed-off-by: Frode Nordahl Needed-By: I95a0d5f1b7aef985df5625cd83222799db811f2b Change-Id: I8a915dca1410c70bdfe7a2d72931921d2a1a265e --- .../drivers/ovn/mech_driver/ovsdb/commands.py | 15 +--- .../ovn/mech_driver/ovsdb/impl_idl_ovn.py | 16 ++++ .../ovn/mech_driver/ovsdb/ovn_client.py | 2 - neutron/tests/unit/fake_resources.py | 1 + .../ovn/mech_driver/ovsdb/test_commands.py | 22 ++--- .../mech_driver/ovsdb/test_impl_idl_ovn.py | 85 +++++++++++++++++-- .../tests/unit/services/ovn_l3/test_plugin.py | 3 - 7 files changed, 111 insertions(+), 33 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 10cdc3f031c..b3a7bdca807 100644 --- a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/commands.py +++ b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/commands.py @@ -836,19 +836,8 @@ class DeleteLRouterExtGwCommand(command.BaseCommand): lrouter.delvalue('nat', nat) nat.delete() - lrouter_ext_ids = getattr(lrouter, 'external_ids', {}) - gw_port_id = lrouter_ext_ids.get(ovn_const.OVN_GW_PORT_EXT_ID_KEY) - if not gw_port_id: - return - - try: - lrouter_port = idlutils.row_by_value( - self.api.idl, 'Logical_Router_Port', 'name', - utils.ovn_lrouter_port_name(gw_port_id)) - except idlutils.RowNotFound: - return - - lrouter.delvalue('ports', lrouter_port) + for gw_port in self.api.get_lrouter_gw_ports(lrouter.name): + lrouter.delvalue('ports', gw_port) class SetLSwitchPortToVirtualTypeCommand(command.BaseCommand): 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 be5e7db3673..8c74661c154 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 @@ -785,6 +785,22 @@ class OvsdbNbOvnIdl(nb_impl_idl.OvnNbApiIdlImpl, Backend): result = lrp.execute(check_error=True) return result[0] if result else None + def get_lrouter_gw_ports(self, lrouter_name): + lr = self.get_lrouter(lrouter_name) + gw_ports = [] + for lrp in getattr(lr, 'ports', []): + lrp_ext_ids = getattr(lrp, 'external_ids', {}) + if (ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY not in lrp_ext_ids or + utils.ovn_name(lrp_ext_ids[ + ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY]) != lr.name): + continue + lrp_ha_cfg = (getattr(lrp, 'gateway_chassis', None) or + getattr(lrp, 'options', {}).get( + ovn_const.OVN_GATEWAY_CHASSIS_KEY)) + if lrp_ha_cfg: + gw_ports.append(lrp) + return gw_ports + def delete_lrouter_ext_gw(self, lrouter_name, if_exists=True): return cmd.DeleteLRouterExtGwCommand(self, lrouter_name, if_exists) 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 f5607e28493..01424b758fa 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 @@ -1371,8 +1371,6 @@ class OVNClient(object): return { ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY: router.get('name', 'no_router_name'), - ovn_const.OVN_GW_PORT_EXT_ID_KEY: - router.get('gw_port_id') or '', ovn_const.OVN_REV_NUM_EXT_ID_KEY: str(utils.get_revision_number( router, ovn_const.TYPE_ROUTERS)), ovn_const.OVN_AZ_HINTS_EXT_ID_KEY: diff --git a/neutron/tests/unit/fake_resources.py b/neutron/tests/unit/fake_resources.py index 2e2737fe632..ff2e0c54017 100644 --- a/neutron/tests/unit/fake_resources.py +++ b/neutron/tests/unit/fake_resources.py @@ -163,6 +163,7 @@ class FakeOvsdbNbOvnIdl(object): self.ha_chassis_group_del = mock.Mock() self.ha_chassis_group_add_chassis = mock.Mock() self.ha_chassis_group_del_chassis = mock.Mock() + self.get_lrouter_gw_ports = 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 39bf2a2334b..864099f3b88 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 @@ -1214,6 +1214,7 @@ class TestDeleteLRouterExtGwCommand(TestBaseCommand): fake_lrouter = fakes.FakeOvsdbRow.create_one_ovsdb_row( attrs={'static_routes': [fake_route_1, fake_route_2], 'nat': []}) + self.ovn_api.get_lrouter_gw_ports.return_value = [] with mock.patch.object(self.ovn_api, "is_col_present", return_value=True): with mock.patch.object(idlutils, 'row_by_value', @@ -1234,6 +1235,7 @@ class TestDeleteLRouterExtGwCommand(TestBaseCommand): fake_lrouter = fakes.FakeOvsdbRow.create_one_ovsdb_row( attrs={'nat': [fake_nat_1, fake_nat_2], 'static_routes': []}) + self.ovn_api.get_lrouter_gw_ports.return_value = [] with mock.patch.object(self.ovn_api, "is_col_present", return_value=True): with mock.patch.object(idlutils, 'row_by_value', @@ -1246,10 +1248,11 @@ class TestDeleteLRouterExtGwCommand(TestBaseCommand): def test_delete_lrouter_extgw_ports(self): port_id = 'fake-port-id' + fake_lrp = fakes.FakeOvsdbRow.create_one_ovsdb_row( + attrs={'gateway_chassis': ['fake_gwc']}) fake_lrouter = fakes.FakeOvsdbRow.create_one_ovsdb_row( - attrs={'external_ids': - {ovn_const.OVN_GW_PORT_EXT_ID_KEY: port_id}, - 'static_routes': [], 'nat': []}) + attrs={'ports': [fake_lrp], 'static_routes': [], 'nat': []}) + self.ovn_api.get_lrouter_gw_ports.return_value = [fake_lrp] with mock.patch.object(self.ovn_api, "is_col_present", return_value=True): with mock.patch.object(idlutils, 'row_by_value', @@ -1258,22 +1261,21 @@ class TestDeleteLRouterExtGwCommand(TestBaseCommand): self.ovn_api, fake_lrouter.name, False) cmd.run_idl(self.transaction) fake_lrouter.delvalue.assert_called_once_with( - 'ports', port_id) + 'ports', fake_lrp) def test_delete_lrouter_extgw_ports_not_found(self): - port_id = 'fake-port-id' fake_lrouter = fakes.FakeOvsdbRow.create_one_ovsdb_row( - attrs={'external_ids': - {ovn_const.OVN_GW_PORT_EXT_ID_KEY: port_id}, - 'static_routes': [], 'nat': []}) + attrs={'static_routes': [], 'nat': []}) + self.ovn_api.get_lrouter_gw_ports.return_value = [] with mock.patch.object(self.ovn_api, "is_col_present", return_value=True): with mock.patch.object(idlutils, 'row_by_value', - side_effect=[fake_lrouter, - idlutils.RowNotFound]): + side_effect=[fake_lrouter]): cmd = commands.DeleteLRouterExtGwCommand( self.ovn_api, fake_lrouter.name, False) cmd.run_idl(self.transaction) + self.ovn_api.get_lrouter_gw_ports.assert_called_once_with( + fake_lrouter.name) fake_lrouter.delvalue.assert_not_called() def _test_delete_lrouter_no_lrouter_exist(self, if_exists=True): diff --git a/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_impl_idl_ovn.py b/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_impl_idl_ovn.py index 209a41bf98c..28546723890 100644 --- a/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_impl_idl_ovn.py +++ b/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_impl_idl_ovn.py @@ -161,7 +161,10 @@ class TestNBImplIdlOvn(TestDBImplIdlOvn): 'lr-name-d'}}, {'name': utils.ovn_name('lr-id-e'), 'external_ids': {ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY: - 'lr-name-e'}}], + 'lr-name-e'}}, + {'name': utils.ovn_name('lr-id-f'), + 'external_ids': {ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY: + 'lr-name-f'}}], 'lrouter_ports': [ {'name': utils.ovn_lrouter_port_name('orp-id-a1'), 'external_ids': {ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY: @@ -169,10 +172,14 @@ class TestNBImplIdlOvn(TestDBImplIdlOvn): 'networks': ['10.0.1.0/24'], 'options': {ovn_const.OVN_GATEWAY_CHASSIS_KEY: 'host-1'}}, {'name': utils.ovn_lrouter_port_name('orp-id-a2'), - 'external_ids': {}, 'networks': ['10.0.2.0/24'], + 'external_ids': {ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY: + 'lr-id-a'}, + 'networks': ['10.0.2.0/24'], 'options': {ovn_const.OVN_GATEWAY_CHASSIS_KEY: 'host-1'}}, {'name': utils.ovn_lrouter_port_name('orp-id-a3'), - 'external_ids': {}, 'networks': ['10.0.3.0/24'], + 'external_ids': {ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY: + 'lr-id-a'}, + 'networks': ['10.0.3.0/24'], 'options': {ovn_const.OVN_GATEWAY_CHASSIS_KEY: ovn_const.OVN_GATEWAY_INVALID_CHASSIS}}, {'name': 'xrp-id-b1', @@ -182,7 +189,15 @@ class TestNBImplIdlOvn(TestDBImplIdlOvn): 'options': {ovn_const.OVN_GATEWAY_CHASSIS_KEY: 'host-2'}}, {'name': utils.ovn_lrouter_port_name('orp-id-b3'), 'external_ids': {}, 'networks': ['20.0.3.0/24'], + 'options': {}}, + {'name': utils.ovn_lrouter_port_name('gwc'), + 'external_ids': {ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY: + 'lr-id-f'}, + 'networks': ['10.0.4.0/24'], 'options': {}}], + 'gateway_chassis': [ + {'chassis_name': 'fake-chassis', + 'name': utils.ovn_lrouter_port_name('gwc') + '_fake-chassis'}], 'static_routes': [{'ip_prefix': '20.0.0.0/16', 'nexthop': '10.0.3.253'}, {'ip_prefix': '10.0.0.0/16', @@ -317,7 +332,12 @@ class TestNBImplIdlOvn(TestDBImplIdlOvn): utils.ovn_lrouter_port_name('orp-id-a3')], utils.ovn_name('lr-id-b'): [ 'xrp-id-b1', - utils.ovn_lrouter_port_name('orp-id-b2')]}, + utils.ovn_lrouter_port_name('orp-id-b2')], + utils.ovn_name('lr-id-f'): [ + utils.ovn_lrouter_port_name('gwc')]}, + 'lrptogwc': { + utils.ovn_lrouter_port_name('gwc'): [ + utils.ovn_lrouter_port_name('gwc') + '_fake-chassis']}, 'lrtosroute': { utils.ovn_name('lr-id-a'): ['20.0.0.0/16'], utils.ovn_name('lr-id-b'): ['10.0.0.0/16'] @@ -346,6 +366,7 @@ class TestNBImplIdlOvn(TestDBImplIdlOvn): self.dhcp_table = fakes.FakeOvsdbTable.create_one_ovsdb_table() self.address_set_table = fakes.FakeOvsdbTable.create_one_ovsdb_table() self.lb_table = fakes.FakeOvsdbTable.create_one_ovsdb_table() + self.gwc_table = fakes.FakeOvsdbTable.create_one_ovsdb_table() self._tables = {} self._tables['Logical_Switch'] = self.lswitch_table @@ -358,6 +379,7 @@ class TestNBImplIdlOvn(TestDBImplIdlOvn): self._tables['Address_Set'] = self.address_set_table self._tables['Load_Balancer'] = self.lb_table self._tables['NAT'] = self.nat_table + self._tables['Gateway_Chassis'] = self.gwc_table with mock.patch.object(impl_idl_ovn.OvsdbNbOvnIdl, 'from_worker', return_value=mock.Mock()): @@ -379,16 +401,23 @@ class TestNBImplIdlOvn(TestDBImplIdlOvn): TestNBImplIdlOvn.fake_associations['lstolsp'], self.lswitch_table, self.lsp_table, 'name', 'name', 'ports') - # Load Routers and Router Ports + # Load Routers, Router Ports and Gateway Chassis fake_lrouters = TestNBImplIdlOvn.fake_set['lrouters'] self._load_ovsdb_fake_rows(self.lrouter_table, fake_lrouters) fake_lrps = TestNBImplIdlOvn.fake_set['lrouter_ports'] self._load_ovsdb_fake_rows(self.lrp_table, fake_lrps) + fake_gwc = TestNBImplIdlOvn.fake_set['gateway_chassis'] + self._load_ovsdb_fake_rows(self.gwc_table, fake_gwc) # Associate routers and router ports self._construct_ovsdb_references( TestNBImplIdlOvn.fake_associations['lrtolrp'], self.lrouter_table, self.lrp_table, 'name', 'name', 'ports') + # Associate router ports and gateway chassis + self._construct_ovsdb_references( + TestNBImplIdlOvn.fake_associations['lrptogwc'], + self.lrp_table, self.gwc_table, + 'name', 'name', 'gateway_chassis') # Load static routes fake_sroutes = TestNBImplIdlOvn.fake_set['static_routes'] self._load_ovsdb_fake_rows(self.sroute_table, fake_sroutes) @@ -484,6 +513,9 @@ class TestNBImplIdlOvn(TestDBImplIdlOvn): {'name': 'lr-id-d', 'ports': {}, 'static_routes': [], 'snats': [], 'dnat_and_snats': []}, {'name': 'lr-id-e', 'ports': {}, 'static_routes': [], + 'snats': [], 'dnat_and_snats': []}, + {'name': 'lr-id-f', 'static_routes': [], + 'ports': {'gwc': ['10.0.4.0/24']}, 'snats': [], 'dnat_and_snats': []}] self.assertCountEqual(mapping, expected) @@ -556,6 +588,11 @@ class TestNBImplIdlOvn(TestDBImplIdlOvn): def test_get_all_chassis_gateway_bindings(self): self._load_nb_db() + + # NOTE(fnordahl): The `Gateway_Chassis` table being present without + # proper associations fools the test, remove for now. + del(self._tables['Gateway_Chassis']) + bindings = self.nb_ovn_idl.get_all_chassis_gateway_bindings() expected = {'host-1': [utils.ovn_lrouter_port_name('orp-id-a1'), utils.ovn_lrouter_port_name('orp-id-a2')], @@ -574,6 +611,11 @@ class TestNBImplIdlOvn(TestDBImplIdlOvn): def test_get_gateway_chassis_binding(self): self._load_nb_db() + + # NOTE(fnordahl): The `Gateway_Chassis` table being present without + # proper associations fools the test, remove for now. + del(self._tables['Gateway_Chassis']) + chassis = self.nb_ovn_idl.get_gateway_chassis_binding( utils.ovn_lrouter_port_name('orp-id-a1')) self.assertEqual(chassis, ['host-1']) @@ -591,6 +633,11 @@ class TestNBImplIdlOvn(TestDBImplIdlOvn): def test_get_unhosted_gateways(self): self._load_nb_db() + + # NOTE(fnordahl): The `Gateway_Chassis` table being present without + # proper associations fools the test, remove for now. + del(self._tables['Gateway_Chassis']) + # Port physnet-dict port_physnet_dict = { 'orp-id-a1': 'physnet1', # scheduled @@ -626,6 +673,11 @@ class TestNBImplIdlOvn(TestDBImplIdlOvn): def test_get_unhosted_gateways_deleted_physnet(self): self._load_nb_db() + + # NOTE(fnordahl): The `Gateway_Chassis` table being present without + # proper associations fools the test, remove for now. + del(self._tables['Gateway_Chassis']) + # The LRP is on host-2 now router_row = self._find_ovsdb_fake_row(self.lrp_table, 'name', 'lrp-orp-id-a1') @@ -813,6 +865,29 @@ class TestNBImplIdlOvn(TestDBImplIdlOvn): lb = self.nb_ovn_idl.get_floatingip_in_nat_or_lb(fip_id) self.assertEqual(lb['_uuid'], lb_row.uuid) + def test_get_lrouter_gw_ports_legacy_option(self): + self._load_nb_db() + + gw1_row = self._find_ovsdb_fake_row( + self.lrp_table, 'name', utils.ovn_lrouter_port_name('orp-id-a1')) + gw2_row = self._find_ovsdb_fake_row( + self.lrp_table, 'name', utils.ovn_lrouter_port_name('orp-id-a2')) + gw3_row = self._find_ovsdb_fake_row( + self.lrp_table, 'name', utils.ovn_lrouter_port_name('orp-id-a3')) + + gw_ports = self.nb_ovn_idl.get_lrouter_gw_ports( + utils.ovn_name('lr-id-a')) + self.assertEqual([gw1_row, gw2_row, gw3_row], gw_ports) + + def test_get_lrouter_gw_ports_gwc(self): + self._load_nb_db() + gw1_row = self._find_ovsdb_fake_row( + self.lrp_table, 'name', utils.ovn_lrouter_port_name('gwc')) + + gw_ports = self.nb_ovn_idl.get_lrouter_gw_ports( + utils.ovn_name('lr-id-f')) + self.assertEqual([gw1_row], gw_ports) + class TestSBImplIdlOvnBase(TestDBImplIdlOvn): diff --git a/neutron/tests/unit/services/ovn_l3/test_plugin.py b/neutron/tests/unit/services/ovn_l3/test_plugin.py index 29a81f22d7c..911cb97fbef 100644 --- a/neutron/tests/unit/services/ovn_l3/test_plugin.py +++ b/neutron/tests/unit/services/ovn_l3/test_plugin.py @@ -434,7 +434,6 @@ class TestOVNL3RouterPlugin(test_mech_driver.Ml2PluginV2TestCase): {'router': updated_data}) self.l3_inst._nb_ovn.update_lrouter.assert_called_once_with( 'neutron-router-id', enabled=True, external_ids={ - ovn_const.OVN_GW_PORT_EXT_ID_KEY: '', ovn_const.OVN_GW_NETWORK_EXT_ID_KEY: '', ovn_const.OVN_REV_NUM_EXT_ID_KEY: '1', ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY: 'router', @@ -456,7 +455,6 @@ class TestOVNL3RouterPlugin(test_mech_driver.Ml2PluginV2TestCase): 'neutron-router-id', enabled=False, external_ids={ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY: 'test', ovn_const.OVN_REV_NUM_EXT_ID_KEY: '1', - ovn_const.OVN_GW_PORT_EXT_ID_KEY: '', ovn_const.OVN_GW_NETWORK_EXT_ID_KEY: '', ovn_const.OVN_AZ_HINTS_EXT_ID_KEY: ''}) @@ -551,7 +549,6 @@ class TestOVNL3RouterPlugin(test_mech_driver.Ml2PluginV2TestCase): external_ids = {ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY: 'router', ovn_const.OVN_REV_NUM_EXT_ID_KEY: '1', - ovn_const.OVN_GW_PORT_EXT_ID_KEY: 'gw-port-id', ovn_const.OVN_GW_NETWORK_EXT_ID_KEY: 'ext-network-id', ovn_const.OVN_AZ_HINTS_EXT_ID_KEY: ''} self.l3_inst._nb_ovn.create_lrouter.assert_called_once_with(