From 45ae9dfb7d5acacc72fcf9f071a9db1beb0ca972 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20J=C3=B3zefczyk?= Date: Mon, 20 Jan 2020 09:39:21 +0000 Subject: [PATCH] [OVN] Delete NAT entry first on any FIP update For now while updating FIP check if port or logical_ip has changed and only then we deleted the NAT entry. Unfortunately each time when FIP update occurs the method _create_or_update_floatingip() is used. It first deletes LSP pointed by FIP and adds it again along with new NAT entries. Based on author comment this actions are required. So if we don't update FIP with logical_ip or new port_id, like update a description, the NAT entries gets duplicated. Since all is wrapped withing a transaction and to not wait for proper fix (this code need sa refactor based on commments with NAT external_id column) I think thats safe just to delete the NAT entry in such situation like described above. Change-Id: Iea532e2a02b7992305d1b90aa040e064901c340c Related-Bug: #1859977 --- .../ovn/mech_driver/ovsdb/ovn_client.py | 7 +---- .../tests/unit/services/ovn_l3/test_plugin.py | 30 +++++++++++++++---- 2 files changed, 26 insertions(+), 11 deletions(-) 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 2082f19ceb0..14a718f7510 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 @@ -936,15 +936,10 @@ class OVNClient(object): floatingip['id'], floatingip, ovn_const.TYPE_FLOATINGIPS) with self._nb_idl.transaction(check_error=True) as txn: txn.add(check_rev_cmd) - if (ovn_fip and - (floatingip['fixed_ip_address'] != ovn_fip['logical_ip'] or - floatingip['port_id'] != ovn_fip['external_ids'].get( - ovn_const.OVN_FIP_PORT_EXT_ID_KEY))): - + if ovn_fip: lrouter = ovn_fip['external_ids'].get( ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY, utils.ovn_name(router_id)) - self._delete_floatingip(ovn_fip, lrouter, txn=txn) fip_status = const.FLOATINGIP_STATUS_DOWN diff --git a/neutron/tests/unit/services/ovn_l3/test_plugin.py b/neutron/tests/unit/services/ovn_l3/test_plugin.py index daa402b2c76..ee7fbbf0296 100644 --- a/neutron/tests/unit/services/ovn_l3/test_plugin.py +++ b/neutron/tests/unit/services/ovn_l3/test_plugin.py @@ -1135,14 +1135,34 @@ class TestOVNL3RouterPlugin(test_mech_driver.Ml2PluginV2TestCase): @mock.patch('neutron.db.l3_db.L3_NAT_dbonly_mixin._get_floatingip') @mock.patch('neutron.db.extraroute_db.ExtraRoute_dbonly_mixin.' 'update_floatingip') - def test_update_floatingip_association_not_changed(self, uf, gf): - self.fake_floating_ip.update({'fixed_port_id': None}) - self.fake_floating_ip_new.update({'port_id': None}) + def test_update_floatingip_association_empty_update(self, uf, gf): + self.get_a_ctx_mock_p.stop() + self.l3_inst._ovn.is_col_present.return_value = True + self.l3_inst._ovn.get_floatingip.return_value = ( + self.fake_ovn_nat_rule) + self.fake_floating_ip.update({'fixed_port_id': 'foo'}) + self.fake_floating_ip_new.update({'port_id': 'foo'}) gf.return_value = self.fake_floating_ip uf.return_value = self.fake_floating_ip_new self.l3_inst.update_floatingip(self.context, 'id', 'floatingip') - self.l3_inst._ovn.delete_nat_rule_in_lrouter.assert_not_called() - self.l3_inst._ovn.add_nat_rule_in_lrouter.assert_not_called() + self.l3_inst._ovn.delete_nat_rule_in_lrouter.assert_called_once_with( + 'neutron-router-id', + type='dnat_and_snat', + logical_ip='10.0.0.10', + external_ip='192.168.0.10') + expected_ext_ids = { + ovn_const.OVN_FIP_EXT_ID_KEY: self.fake_floating_ip_new['id'], + ovn_const.OVN_REV_NUM_EXT_ID_KEY: '1', + ovn_const.OVN_FIP_PORT_EXT_ID_KEY: + self.fake_floating_ip_new['port_id'], + ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY: utils.ovn_name( + self.fake_floating_ip_new['router_id'])} + self.l3_inst._ovn.add_nat_rule_in_lrouter.assert_called_once_with( + 'neutron-new-router-id', + type='dnat_and_snat', + logical_ip='10.10.10.10', + external_ip='192.168.0.10', + external_ids=expected_ext_ids) @mock.patch('neutron.db.l3_db.L3_NAT_dbonly_mixin._get_floatingip') @mock.patch('neutron.db.extraroute_db.ExtraRoute_dbonly_mixin.'