From b9b819d7665ee72fd3fd86b1e08f2121451e6c94 Mon Sep 17 00:00:00 2001 From: Luis Tomas Bolivar Date: Wed, 28 Jun 2023 12:18:30 +0200 Subject: [PATCH] Ensure traffic is not centralized if DVR is enabled There is no need to clear the external_mac if DVR is enabled, not even when the port is down. This patch ensures the external_mac is only deleted when DVR is not enabled. Without this patch, if a VM with a floating IP gets deleted, and DVR is enabled, during some time the traffic gets (wrongly) centralized while it should not. And it is also generating more load on the OVN side unnecesarily. Closes-Bug: #2025264 Change-Id: I89db15dd1b629bc963f3b63926391a4a02cbedf7 (cherry picked from commit 0090572b93d96348778c724194c26a976a9f8757) --- .../drivers/ovn/mech_driver/mech_driver.py | 10 ++++---- .../ovn/mech_driver/test_mech_driver.py | 23 +++++++++++-------- .../dvr-external-mac-934409413e515eb2.yaml | 10 ++++++++ 3 files changed, 28 insertions(+), 15 deletions(-) create mode 100644 releasenotes/notes/dvr-external-mac-934409413e515eb2.yaml 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 735281b9e93..59cdfe91c91 100644 --- a/neutron/plugins/ml2/drivers/ovn/mech_driver/mech_driver.py +++ b/neutron/plugins/ml2/drivers/ovn/mech_driver/mech_driver.py @@ -1048,7 +1048,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 @@ -1069,9 +1069,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( @@ -1137,7 +1137,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 c638553c1fa..6311fbe3833 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 @@ -1124,7 +1124,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 @@ -1174,7 +1174,7 @@ def test_set_port_status_concurrent_delete(self): resources.PORT, provisioning_blocks.L2_AGENT_ENTITY ) - ude.assert_called_once_with(port1['port']['id'], False) + ude.assert_called_once_with(port1['port']['id']) def test_bind_port_unsupported_vnic_type(self): fake_port = fakes.FakePort.create_one_port( @@ -2368,9 +2368,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() @@ -2383,22 +2384,24 @@ def _test__update_dnat_entry_if_needed(self, up=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, up=up) + self.mech_driver._update_dnat_entry_if_needed(port_id) - if up: + 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') diff --git a/releasenotes/notes/dvr-external-mac-934409413e515eb2.yaml b/releasenotes/notes/dvr-external-mac-934409413e515eb2.yaml new file mode 100644 index 00000000000..fd4b8b5f7b8 --- /dev/null +++ b/releasenotes/notes/dvr-external-mac-934409413e515eb2.yaml @@ -0,0 +1,10 @@ +--- +other: + - | + The external_mac entry in the NAT table is used to distribute/centralize + the traffic to the FIPs. When there is an external_mac set the traffic + is distributed (DVR). When it is empty it is centralized through the + gateway port (no DVR). Upon port status transition to down, the + external_mac was removed regardless of DVR being enabled or not, leading + to centralize the FIP traffic for DVR -- though it was for down ports that + won't accept traffic anyway.