diff --git a/ovn_bgp_agent/drivers/openstack/utils/wire.py b/ovn_bgp_agent/drivers/openstack/utils/wire.py index b364d0e0..89e35e82 100644 --- a/ovn_bgp_agent/drivers/openstack/utils/wire.py +++ b/ovn_bgp_agent/drivers/openstack/utils/wire.py @@ -512,19 +512,37 @@ def cleanup_wiring(idl, bridge_mappings, ovs_flows, exposed_ips, elif CONF.exposing_method == constants.EXPOSE_METHOD_VRF: return _cleanup_wiring_evpn(ovs_flows, routing_tables_routes) elif CONF.exposing_method == constants.EXPOSE_METHOD_OVN: - # TODO(ltomasbo): clean up old policies, routes and proxy_arps cidrs - return True + return _cleanup_wiring_ovn(exposed_ips) + + +def _cleanup_extra_ips(exposed_ips: list[str]) -> set[str]: + current_ips = set(linux_net.get_exposed_ips(CONF.bgp_nic)) + expected_ips = {ip for ip_dict in exposed_ips.values() + for ip in ip_dict.keys()} + ips_to_delete = current_ips - expected_ips + if ips_to_delete: + LOG.warning(f"Removing IPs {ips_to_delete} from {CONF.bgp_nic} as " + "they are not expected to be exposed.") + linux_net.delete_exposed_ips(ips_to_delete, CONF.bgp_nic) + return expected_ips + + +def _cleanup_wiring_ovn(exposed_ips): + """Cleanup IPs that are not expected to be exposed on the bgp_nic. + + This may be needed in case ovn-bgp-agent misses updates from the OVN NB + database due to temporary downtime, networking issues between the + ovn-bgp-agent and the NB DB or otherwise. + """ + _cleanup_extra_ips(exposed_ips) + # TODO(dmitriis): clean up other state: in the local OVN NB: logical + # router policies, logical router routes, ARP proxies in logical switch + # ports, static mac bindings. def _cleanup_wiring_underlay(idl, bridge_mappings, ovs_flows, exposed_ips, routing_tables, routing_tables_routes): - current_ips = linux_net.get_exposed_ips(CONF.bgp_nic) - expected_ips = [ip for ip_dict in exposed_ips.values() - for ip in ip_dict.keys()] - - ips_to_delete = [ip for ip in current_ips if ip not in expected_ips] - linux_net.delete_exposed_ips(ips_to_delete, CONF.bgp_nic) - + expected_ips = _cleanup_extra_ips(exposed_ips) extra_routes = {} for bridge in bridge_mappings.values(): extra_routes[bridge] = ( diff --git a/ovn_bgp_agent/tests/unit/drivers/openstack/test_nb_ovn_bgp_driver.py b/ovn_bgp_agent/tests/unit/drivers/openstack/test_nb_ovn_bgp_driver.py index 1848c49a..d3ca7664 100644 --- a/ovn_bgp_agent/tests/unit/drivers/openstack/test_nb_ovn_bgp_driver.py +++ b/ovn_bgp_agent/tests/unit/drivers/openstack/test_nb_ovn_bgp_driver.py @@ -230,7 +230,7 @@ class TestNBOVNBGPDriver(test_base.TestCase): mock_expose_ovn_lb_vip.assert_called_once_with(lb1) mock_expose_ovn_lb_fip.assert_called_once_with(lb1) mock_del_exposed_ips.assert_called_once_with( - ips, CONF.bgp_nic) + set(ips), CONF.bgp_nic) mock_del_ip_rules.assert_called_once_with(fake_ip_rules) mock_del_ip_routes.assert_called_once() bridge = set(self.nb_bgp_driver.ovn_bridge_mappings.values()).pop() diff --git a/ovn_bgp_agent/tests/unit/drivers/openstack/utils/test_wire.py b/ovn_bgp_agent/tests/unit/drivers/openstack/utils/test_wire.py index 5a77b108..9f99772c 100644 --- a/ovn_bgp_agent/tests/unit/drivers/openstack/utils/test_wire.py +++ b/ovn_bgp_agent/tests/unit/drivers/openstack/utils/test_wire.py @@ -328,18 +328,27 @@ class TestWire(test_base.TestCase): self.sb_idl, self.bridge_mappings, ovs_flows, exposed_ips, routing_tables, routing_tables_routes) - def test_cleanup_wiring_ovn(self): + @mock.patch.object(linux_net, 'get_exposed_ips') + @mock.patch.object(linux_net, 'delete_exposed_ips') + def test_cleanup_wiring_ovn(self, _delete_exposed_ips, _get_exposed_ips): CONF.set_override('exposing_method', 'ovn') self.addCleanup(CONF.clear_override, 'exposing_method') + # Both IPs are present on the NIC but .42 is not expected + # to be exposed. + _get_exposed_ips.return_value = ['192.0.2.42', '192.0.2.24'] + ovs_flows = {} - exposed_ips = {} + exposed_ips = { + "fakels": {"192.0.2.24": {'bridge_device': "br-example"}}} routing_tables = {} routing_tables_routes = {} - ret = wire.cleanup_wiring(self.sb_idl, self.bridge_mappings, ovs_flows, - exposed_ips, routing_tables, - routing_tables_routes) - self.assertTrue(ret) + wire.cleanup_wiring(self.sb_idl, self.bridge_mappings, ovs_flows, + exposed_ips, routing_tables, + routing_tables_routes) + # Make sure we only delete the IP that isn't supposed to be exposed. + _delete_exposed_ips.assert_called_once_with({"192.0.2.42"}, + CONF.bgp_nic) def test_cleanup_wiring_evpn(self): CONF.set_override('exposing_method', 'vrf')