From 2acbbcfb83d0ec24aaedbedc5738074c6b6a855f Mon Sep 17 00:00:00 2001 From: Matt Crees Date: Tue, 14 Nov 2023 11:12:43 +0000 Subject: [PATCH 1/2] Revert: Revert: Ensure traffic is not centralized if DVR is enabled --- .../drivers/ovn/mech_driver/mech_driver.py | 10 +++---- .../ovn/mech_driver/test_mech_driver.py | 28 ++++++++++++------- 2 files changed, 23 insertions(+), 15 deletions(-) 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 c427475b6ae..d2085ef7284 100644 --- a/neutron/plugins/ml2/drivers/ovn/mech_driver/mech_driver.py +++ b/neutron/plugins/ml2/drivers/ovn/mech_driver/mech_driver.py @@ -1056,7 +1056,7 @@ def get_workers(self): # See doc/source/design/ovn_worker.rst for more details. return [worker.MaintenanceWorker()] - def _update_dnat_entry_if_needed(self, port_id, up=True): + def _update_dnat_entry_if_needed(self, port_id): """Update DNAT entry if using distributed floating ips.""" if not self.nb_ovn: self.nb_ovn = self._ovn_client._nb_idl @@ -1077,9 +1077,9 @@ def _update_dnat_entry_if_needed(self, port_id, up=True): {ovn_const.OVN_FIP_EXT_MAC_KEY: nat['external_mac']})).execute() - if up and ovn_conf.is_ovn_distributed_floating_ip(): - mac = nat['external_ids'][ovn_const.OVN_FIP_EXT_MAC_KEY] - if nat['external_mac'] != mac: + if ovn_conf.is_ovn_distributed_floating_ip(): + mac = nat['external_ids'].get(ovn_const.OVN_FIP_EXT_MAC_KEY) + if mac and nat['external_mac'] != mac: LOG.debug("Setting external_mac of port %s to %s", port_id, mac) self.nb_ovn.db_set( @@ -1147,7 +1147,7 @@ def set_port_status_down(self, port_id): # to prevent another entity from bypassing the block with its own # port status update. LOG.info("OVN reports status down for port: %s", port_id) - self._update_dnat_entry_if_needed(port_id, False) + self._update_dnat_entry_if_needed(port_id) admin_context = n_context.get_admin_context() try: db_port = ml2_db.get_port(admin_context, port_id) 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 3d84de31876..bceedf3fd7a 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 @@ -1130,7 +1130,7 @@ def _test_set_port_status_down(self, is_compute_port=False): resources.PORT, provisioning_blocks.L2_AGENT_ENTITY ) - ude.assert_called_once_with(port1['port']['id'], False) + ude.assert_called_once_with(port1['port']['id']) # If the port does NOT bellong to compute, do not notify Nova # about it's status changes @@ -1175,7 +1175,7 @@ def test_set_port_status_concurrent_delete(self): side_effect=exc) as apc, \ mock.patch.object(self.mech_driver, '_update_dnat_entry_if_needed') as ude: - self.mech_driver.set_port_status_down(port1['port']['id'], False) + self.mech_driver.set_port_status_down(port1['port']['id']) apc.assert_called_once_with( mock.ANY, port1['port']['id'], @@ -2376,9 +2376,10 @@ def test_agent_with_nb_cfg_timestamp_not_timeout(self): self.assertTrue(agent.alive, "Agent of type %s alive=%s" % ( agent.agent_type, agent.alive)) - def _test__update_dnat_entry_if_needed(self, up=True): - ovn_conf.cfg.CONF.set_override( - 'enable_distributed_floating_ip', True, group='ovn') + def _test__update_dnat_entry_if_needed(self, dvr=True): + if dvr: + ovn_conf.cfg.CONF.set_override( + 'enable_distributed_floating_ip', True, group='ovn') port_id = 'fake-port-id' fake_ext_mac_key = 'fake-ext-mac-key' fake_nat_uuid = uuidutils.generate_uuid() @@ -2386,22 +2387,29 @@ def _test__update_dnat_entry_if_needed(self, up=True): attrs={'_uuid': fake_nat_uuid, 'external_ids': { ovn_const.OVN_FIP_EXT_MAC_KEY: fake_ext_mac_key}, 'external_mac': 'aa:aa:aa:aa:aa:aa'}) + fake_db_find = mock.Mock() fake_db_find.execute.return_value = [nat_row] self.nb_ovn.db_find.return_value = fake_db_find - self.mech_driver._update_dnat_entry_if_needed(port_id, up=up) - if up: + + self.mech_driver._update_dnat_entry_if_needed(port_id) + + if dvr: # Assert that we are setting the external_mac in the NAT table self.nb_ovn.db_set.assert_called_once_with( 'NAT', fake_nat_uuid, ('external_mac', fake_ext_mac_key)) + self.nb_ovn.db_clear.assert_not_called() else: + self.nb_ovn.db_set.assert_not_called() # Assert that we are cleaning the external_mac from the NAT table self.nb_ovn.db_clear.assert_called_once_with( 'NAT', fake_nat_uuid, 'external_mac') - def test__update_dnat_entry_if_needed_up(self): + + def test__update_dnat_entry_if_needed_dvr(self): self._test__update_dnat_entry_if_needed() - def test__update_dnat_entry_if_needed_down(self): - self._test__update_dnat_entry_if_needed(up=False) + + def test__update_dnat_entry_if_needed_no_dvr(self): + self._test__update_dnat_entry_if_needed(dvr=False) @mock.patch('neutron.plugins.ml2.drivers.ovn.mech_driver.ovsdb.' 'ovn_client.OVNClient._get_router_ports') From 5726a8c196f46738f67e8e4f9d0f8b1437ec3bc7 Mon Sep 17 00:00:00 2001 From: Luis Tomas Bolivar Date: Mon, 13 Nov 2023 16:42:51 +0100 Subject: [PATCH 2/2] Ensure ovn loadbalancer FIPs are centralized upon neutron restarts When neutron server restarts the mac address for NAT entries related to ovn-lb FIPs gets re-added, distributing the traffic that should be centralized and therefore breaking the connectivity. This happens due to the port being down. This patch is ensuring the MAC entry is only being readded in case the port is UP Closes-Bug: #2042938 Change-Id: I6203009750a4e589eeb808f842cb522d61476179 --- .../drivers/ovn/mech_driver/mech_driver.py | 19 +++++------ .../ovn/mech_driver/test_mech_driver.py | 32 ++++++++++++------- 2 files changed, 30 insertions(+), 21 deletions(-) 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 d2085ef7284..706518ce1bf 100644 --- a/neutron/plugins/ml2/drivers/ovn/mech_driver/mech_driver.py +++ b/neutron/plugins/ml2/drivers/ovn/mech_driver/mech_driver.py @@ -1056,7 +1056,7 @@ def get_workers(self): # See doc/source/design/ovn_worker.rst for more details. return [worker.MaintenanceWorker()] - def _update_dnat_entry_if_needed(self, port_id): + def _update_dnat_entry_if_needed(self, port_id, up=True): """Update DNAT entry if using distributed floating ips.""" if not self.nb_ovn: self.nb_ovn = self._ovn_client._nb_idl @@ -1078,13 +1078,14 @@ def _update_dnat_entry_if_needed(self, port_id): nat['external_mac']})).execute() if ovn_conf.is_ovn_distributed_floating_ip(): - mac = nat['external_ids'].get(ovn_const.OVN_FIP_EXT_MAC_KEY) - if mac and nat['external_mac'] != mac: - LOG.debug("Setting external_mac of port %s to %s", - port_id, mac) - self.nb_ovn.db_set( - 'NAT', nat['_uuid'], ('external_mac', mac)).execute( - check_error=True) + if up: + mac = nat['external_ids'].get(ovn_const.OVN_FIP_EXT_MAC_KEY) + if mac and nat['external_mac'] != mac: + LOG.debug("Setting external_mac of port %s to %s", + port_id, mac) + self.nb_ovn.db_set( + 'NAT', nat['_uuid'], ('external_mac', mac)).execute( + check_error=True) else: if nat['external_mac']: LOG.debug("Clearing up external_mac of port %s", port_id) @@ -1147,7 +1148,7 @@ def set_port_status_down(self, port_id): # to prevent another entity from bypassing the block with its own # port status update. LOG.info("OVN reports status down for port: %s", port_id) - self._update_dnat_entry_if_needed(port_id) + self._update_dnat_entry_if_needed(port_id, False) admin_context = n_context.get_admin_context() try: db_port = ml2_db.get_port(admin_context, port_id) 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 bceedf3fd7a..e1c39a5ea61 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 @@ -1130,7 +1130,7 @@ def _test_set_port_status_down(self, is_compute_port=False): resources.PORT, provisioning_blocks.L2_AGENT_ENTITY ) - ude.assert_called_once_with(port1['port']['id']) + ude.assert_called_once_with(port1['port']['id'], False) # If the port does NOT bellong to compute, do not notify Nova # about it's status changes @@ -1182,7 +1182,7 @@ def test_set_port_status_concurrent_delete(self): resources.PORT, provisioning_blocks.L2_AGENT_ENTITY ) - ude.assert_called_once_with(port1['port']['id']) + ude.assert_called_once_with(port1['port']['id'], False) def test_bind_port_unsupported_vnic_type(self): fake_port = fakes.FakePort.create_one_port( @@ -2376,7 +2376,7 @@ def test_agent_with_nb_cfg_timestamp_not_timeout(self): self.assertTrue(agent.alive, "Agent of type %s alive=%s" % ( agent.agent_type, agent.alive)) - def _test__update_dnat_entry_if_needed(self, dvr=True): + def _test__update_dnat_entry_if_needed(self, up=True, dvr=True): if dvr: ovn_conf.cfg.CONF.set_override( 'enable_distributed_floating_ip', True, group='ovn') @@ -2392,25 +2392,33 @@ def _test__update_dnat_entry_if_needed(self, dvr=True): fake_db_find.execute.return_value = [nat_row] self.nb_ovn.db_find.return_value = fake_db_find - self.mech_driver._update_dnat_entry_if_needed(port_id) + self.mech_driver._update_dnat_entry_if_needed(port_id, up=up) - if dvr: + if up and dvr: # Assert that we are setting the external_mac in the NAT table self.nb_ovn.db_set.assert_called_once_with( 'NAT', fake_nat_uuid, ('external_mac', fake_ext_mac_key)) - self.nb_ovn.db_clear.assert_not_called() else: - self.nb_ovn.db_set.assert_not_called() - # Assert that we are cleaning the external_mac from the NAT table - self.nb_ovn.db_clear.assert_called_once_with( - 'NAT', fake_nat_uuid, 'external_mac') + if dvr: + self.nb_ovn.db_set.assert_not_called() + else: + # Assert that we are cleaning the external_mac from the NAT + # table + self.nb_ovn.db_clear.assert_called_once_with( + 'NAT', fake_nat_uuid, 'external_mac') - def test__update_dnat_entry_if_needed_dvr(self): + def test__update_dnat_entry_if_needed_up_dvr(self): self._test__update_dnat_entry_if_needed() - def test__update_dnat_entry_if_needed_no_dvr(self): + def test__update_dnat_entry_if_needed_up_no_dvr(self): self._test__update_dnat_entry_if_needed(dvr=False) + def test__update_dnat_entry_if_needed_down_dvr(self): + self._test__update_dnat_entry_if_needed(up=False) + + def test__update_dnat_entry_if_needed_down_no_dvr(self): + self._test__update_dnat_entry_if_needed(up=False, dvr=False) + @mock.patch('neutron.plugins.ml2.drivers.ovn.mech_driver.ovsdb.' 'ovn_client.OVNClient._get_router_ports') def _test_update_network_fragmentation(self, new_mtu, expected_opts, grps):