From 7590dff9b17ae8a889fce1dc7a0636a4e58b61de Mon Sep 17 00:00:00 2001 From: Martin Kalcok Date: Wed, 27 Aug 2025 19:17:04 +0200 Subject: [PATCH 1/3] ovn_db_sync: Improve coexistence support. neutron-ovn-db-sync-util synchronizes content between neutron's database and OVN NB/SB databases. As a side-effect, it can sometimes remove resources from OVN database that were not meant for neutron to manage. Coexistence support [0] aims to avoid these scenarios. The ovn_db_sync script already tries to avoid these unwanted removals by checking for presence of well known neutron external_ids for resources like "Logical Switch" and "Logical Switch Port" [1]. This change adds similar checks for: * Logical Router Port * Static Route * Port Group NAT rules are still missing the check because they don't have "neutron:" external_ids to check. In addition to the ovn_db_sync script, there is a 'maintenance' process that periodically updates OVN resources. This change also update its methods to not alter resources not owned by the neutron. [0] https://specs.openstack.org/openstack/neutron-specs/specs/2024.1/ml2ovn-coexistence-support-ovn-ext-resources.html [1] https://opendev.org/openstack/neutron/src/commit/f9067a719084710ee4f46fa31edb6a938e0dbbb0/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/impl_idl_ovn.py#L308-L316 Closes-bug: #2027742 Change-Id: I1434700928779577073d1369c0a2983a4076cc0e Signed-off-by: Martin Kalcok (cherry picked from commit ff57491a976e0a8b4945f7dd55096370c744c76a) --- .../ovn/mech_driver/ovsdb/impl_idl_ovn.py | 16 ++- .../ovn/mech_driver/ovsdb/maintenance.py | 11 +- .../ovn/mech_driver/ovsdb/ovn_db_sync.py | 6 +- .../ovn/mech_driver/ovsdb/test_ovn_db_sync.py | 122 ++++++++++++++++-- .../mech_driver/ovsdb/test_impl_idl_ovn.py | 33 ++++- .../ovn/mech_driver/ovsdb/test_maintenance.py | 39 ++++-- .../ovn/mech_driver/ovsdb/test_ovn_db_sync.py | 12 +- 7 files changed, 199 insertions(+), 40 deletions(-) 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 7c25b69049e..7282d0fa215 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 @@ -332,11 +332,17 @@ def get_all_logical_routers_with_rports(self): if ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY not in ( lrouter.external_ids): continue - lrports = {lrport.name.replace('lrp-', ''): lrport.networks - for lrport in getattr(lrouter, 'ports', [])} - sroutes = [{'destination': sroute.ip_prefix, - 'nexthop': sroute.nexthop} - for sroute in getattr(lrouter, 'static_routes', [])] + lrports = { + lrport.name.replace('lrp-', ''): lrport.networks + for lrport in getattr(lrouter, 'ports', []) + if ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY in lrport.external_ids + } + sroutes = [ + {'destination': route.ip_prefix, 'nexthop': route.nexthop} + for route in getattr(lrouter, 'static_routes', []) + if any(eid.startswith(constants.DEVICE_OWNER_NEUTRON_PREFIX) + for eid in route.external_ids) + ] dnat_and_snats = [] snat = [] diff --git a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/maintenance.py b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/maintenance.py index 2783ec84102..95ee4c464eb 100644 --- a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/maintenance.py +++ b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/maintenance.py @@ -533,6 +533,8 @@ def check_for_igmp_snoop_support(self): cmds = [] for ls in self._nb_idl.ls_list().execute(check_error=True): + if ovn_const.OVN_NETWORK_NAME_EXT_ID_KEY not in ls.external_ids: + continue snooping = ls.other_config.get(ovn_const.MCAST_SNOOP) flood = ls.other_config.get(ovn_const.MCAST_FLOOD_UNREGISTERED) @@ -572,9 +574,11 @@ def check_for_ha_chassis_group(self): context = n_context.get_admin_context() with self._nb_idl.transaction(check_error=True) as txn: for port in external_ports: - network_id = port.external_ids[ - ovn_const.OVN_NETWORK_NAME_EXT_ID_KEY].replace( + network_id = port.external_ids.get( + ovn_const.OVN_NETWORK_NAME_EXT_ID_KEY, '').replace( ovn_const.OVN_NAME_PREFIX, '') + if not network_id: + continue ha_ch_grp, high_prio_ch = utils.sync_ha_chassis_group_network( context, self._nb_idl, self._sb_idl, port.name, network_id, txn) @@ -1005,6 +1009,9 @@ def set_network_type(self): for seg in net_segments} cmds = [] for ls in self._nb_idl.ls_list().execute(check_error=True): + if ovn_const.OVN_NETWORK_NAME_EXT_ID_KEY not in ls.external_ids: + continue + if ovn_const.OVN_NETTYPE_EXT_ID_KEY not in ls.external_ids: net_id = utils.get_neutron_name(ls.name) external_ids = { diff --git a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_db_sync.py b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_db_sync.py index a88d1c76a06..4901f1fda90 100644 --- a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_db_sync.py +++ b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_db_sync.py @@ -196,7 +196,11 @@ def sync_port_groups(self, ctx): ovn_pgs = set() port_groups = self.ovn_api.db_list_rows('Port_Group').execute() or [] for pg in port_groups: - ovn_pgs.add(pg.name) + # Default neutron "drop pg" does NOT have any external IDs, but + # we still want to manage it, so we match it on its name. + if (ovn_const.OVN_SG_EXT_ID_KEY in pg.external_ids or + pg.name == ovn_const.OVN_DROP_PORT_GROUP_NAME): + ovn_pgs.add(pg.name) add_pgs = neutron_pgs.difference(ovn_pgs) remove_pgs = ovn_pgs.difference(neutron_pgs) diff --git a/neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_ovn_db_sync.py b/neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_ovn_db_sync.py index 6fb51367be3..51fc861a577 100644 --- a/neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_ovn_db_sync.py +++ b/neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_ovn_db_sync.py @@ -98,6 +98,12 @@ def setUp(self, *args): self.expected_dns_records = [] self.expected_ports_with_unknown_addr = [] self.expected_qos_records = [] + # Set of externally managed resources that should not + # be cleaned up by the sync_db + self.create_ext_port_groups = [] + self.create_ext_lrouter_ports = [] + self.create_ext_lrouter_routes = [] + self.ctx = context.get_admin_context() ovn_config.cfg.CONF.set_override('ovn_metadata_enabled', True, group='ovn') @@ -515,6 +521,9 @@ def _create_resources(self, restart_ovsdb_processes=False): self.create_lrouter_routes.append(('neutron-' + r1['id'], '10.13.0.0/24', '20.0.0.13')) + self.create_ext_lrouter_routes.append(('neutron-' + r1['id'], + '10.14.0.0/24', + '20.0.0.14')) self.delete_lrouter_routes.append(('neutron-' + r1['id'], '10.10.0.0/24', '20.0.0.10')) @@ -662,10 +671,18 @@ def _create_resources(self, restart_ovsdb_processes=False): 'neutron-' + r1['id'])) self.create_lrouter_ports.append(('lrp-' + uuidutils.generate_uuid(), 'neutron-' + r1['id'])) + self.create_ext_lrouter_ports.append( + ('ext-lrp-' + uuidutils.generate_uuid(), 'neutron-' + r1['id']) + ) + self.create_ext_lrouter_ports.append( + ('ext-lrp-' + uuidutils.generate_uuid(), 'neutron-' + r1['id']) + ) self.delete_lrouters.append('neutron-' + r2['id']) self.create_port_groups.extend([{'name': 'pg1', 'acls': []}, {'name': 'pg2', 'acls': []}]) + self.create_ext_port_groups.extend([{'name': 'ext-pg1', 'acls': []}, + {'name': 'ext-pg2', 'acls': []}]) self.delete_port_groups.append( utils.ovn_port_group_name(n1_prtr['port']['security_groups'][0])) # Create a network and subnet with orphaned OVN resources. @@ -790,7 +807,14 @@ def _modify_resources_in_nb_db(self): txn.add(self.nb_api.lr_del(lrouter_name, if_exists=True)) for lrport, lrouter_name in self.create_lrouter_ports: - txn.add(self.nb_api.add_lrouter_port(lrport, lrouter_name)) + external_ids = {ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY: + lrouter_name} + txn.add(self.nb_api.add_lrouter_port( + lrport, lrouter_name, True, external_ids=external_ids)) + + for lrport, lrouter_name in self.create_ext_lrouter_ports: + txn.add(self.nb_api.add_lrouter_port( + lrport, lrouter_name, True)) for lrport, lrouter_name, networks in self.update_lrouter_ports: txn.add(self.nb_api.update_lrouter_port( @@ -807,6 +831,10 @@ def _modify_resources_in_nb_db(self): ip_prefix=ip_prefix, nexthop=nexthop, **columns)) + for lr_name, ip_prefix, nexthop in self.create_ext_lrouter_routes: + txn.add(self.nb_api.add_static_route(lr_name, + ip_prefix=ip_prefix, + nexthop=nexthop)) routers = defaultdict(list) for lrouter_name, ip_prefix, nexthop in self.delete_lrouter_routes: routers[lrouter_name].append((ip_prefix, nexthop)) @@ -839,7 +867,12 @@ def _modify_resources_in_nb_db(self): txn.add(self.nb_api.delete_acl(lswitch_name, lport_name, True)) + columns = { + 'external_ids': {ovn_const.OVN_SG_EXT_ID_KEY: 'sg_uuid'}, + } for pg in self.create_port_groups: + txn.add(self.nb_api.pg_add(**pg, **columns)) + for pg in self.create_ext_port_groups: txn.add(self.nb_api.pg_add(**pg)) for pg in self.delete_port_groups: txn.add(self.nb_api.pg_del(pg)) @@ -1310,6 +1343,7 @@ def _get_ipv6_ra_configs_for_router_port(port): self.ctx, port)) return ipv6_ra_configs + neutron_prefix = constants.DEVICE_OWNER_NEUTRON_PREFIX for router_id in db_router_ids: r_ports = self._list('ports', query_params='device_id=%s' % (router_id)) @@ -1328,18 +1362,26 @@ def _get_ipv6_ra_configs_for_router_port(port): lrouter = idlutils.row_by_value( self.mech_driver.nb_ovn.idl, 'Logical_Router', 'name', 'neutron-' + str(router_id), None) - lports = getattr(lrouter, 'ports', []) + all_lports = getattr(lrouter, 'ports', []) + managed_lports = [ + lport for lport in all_lports + if (ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY in + lport.external_ids)] + plugin_lrouter_port_ids = [lport.name.replace('lrp-', '') - for lport in lports] + for lport in managed_lports] plugin_lport_networks = { lport.name.replace('lrp-', ''): lport.networks - for lport in lports} + for lport in managed_lports} plugin_lport_ra_configs = { lport.name.replace('lrp-', ''): lport.ipv6_ra_configs - for lport in lports} + for lport in managed_lports} sroutes = getattr(lrouter, 'static_routes', []) - plugin_routes = [sroute.ip_prefix + sroute.nexthop - for sroute in sroutes] + plugin_routes = [] + for sroute in sroutes: + if any(e_id.startswith(neutron_prefix) + for e_id in sroute.external_ids): + plugin_routes.append(sroute.ip_prefix + sroute.nexthop) nats = getattr(lrouter, 'nat', []) plugin_nats = [ nat.external_ip + nat.logical_ip + nat.type + @@ -1355,18 +1397,28 @@ def _get_ipv6_ra_configs_for_router_port(port): lrouter = idlutils.row_by_value( self.nb_api.idl, 'Logical_Router', 'name', 'neutron-' + router_id, None) - lports = getattr(lrouter, 'ports', []) + all_lports = getattr(lrouter, 'ports', []) + managed_lports = [ + lport for lport in all_lports + if (ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY in + lport.external_ids)] monitor_lrouter_port_ids = [lport.name.replace('lrp-', '') - for lport in lports] + for lport in managed_lports] monitor_lport_networks = { lport.name.replace('lrp-', ''): lport.networks - for lport in lports} + for lport in managed_lports} monitor_lport_ra_configs = { lport.name.replace('lrp-', ''): lport.ipv6_ra_configs - for lport in lports} + for lport in managed_lports} sroutes = getattr(lrouter, 'static_routes', []) - monitor_routes = [sroute.ip_prefix + sroute.nexthop - for sroute in sroutes] + monitor_routes = [] + for sroute in sroutes: + if any(e_id.startswith(neutron_prefix) + for e_id in sroute.external_ids): + monitor_routes.append( + sroute.ip_prefix + sroute.nexthop + ) + nats = getattr(lrouter, 'nat', []) monitor_nats = [ nat.external_ip + nat.logical_ip + nat.type + @@ -1524,7 +1576,9 @@ def _validate_port_groups(self, should_match=True): mn_pgs = [] for row in self.nb_api.tables['Port_Group'].rows.values(): - mn_pgs.append(getattr(row, 'name', '')) + if (ovn_const.OVN_SG_EXT_ID_KEY in row.external_ids or + row.name == ovn_const.OVN_DROP_PORT_GROUP_NAME): + mn_pgs.append(getattr(row, 'name', '')) if should_match: self.assertCountEqual(nb_pgs, db_pgs) @@ -1630,6 +1684,46 @@ def _test_ovn_nb_sync_helper(self, mode, modify_resources=True, self._sync_resources(mode) self._validate_resources(should_match=should_match_after_sync) + if not restart_ovsdb_processes: + # Restarting ovsdb-server removes all its previous content. + # We can not expect to find external resources in the DB + # if it was wiped out. + self._validate_external_resources() + + def _validate_external_resources(self): + """Ensure that resources not owned by Neutron are in the OVN DB. + + This function is useful to validate that external resources survived + ovn_db_sync. + """ + db_routers = self._list('routers') + db_router_ids = [router['id'] for router in db_routers['routers']] + + pgs = [] + for pg in self.nb_api.tables['Port_Group'].rows.values(): + pgs.append(pg.name) + + lrports = [] + sroutes = [] + for router_id in db_router_ids: + lrouter = idlutils.row_by_value( + self.mech_driver.nb_ovn.idl, 'Logical_Router', 'name', + 'neutron-' + str(router_id), None) + + for lrport in getattr(lrouter, 'ports', []): + lrports.append(lrport.name) + + for route in getattr(lrouter, 'static_routes', []): + sroutes.append(route.ip_prefix + route.nexthop) + + for port_name, _ in self.create_ext_lrouter_ports: + self.assertIn(port_name, lrports) + + for _, prefix, next_hop in self.create_ext_lrouter_routes: + self.assertIn(prefix + next_hop, sroutes) + + for ext_pg in self.create_ext_port_groups: + self.assertIn(ext_pg['name'], pgs) def test_ovn_nb_sync_repair(self): self._test_ovn_nb_sync_helper(ovn_const.OVN_DB_SYNC_MODE_REPAIR) 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 130f1ffc453..db9bdd4fbcd 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 @@ -183,25 +183,46 @@ class TestNBImplIdlOvn(TestDBImplIdlOvn): 'networks': ['10.0.3.0/24'], 'options': {ovn_const.OVN_GATEWAY_CHASSIS_KEY: None}}, {'name': 'xrp-id-b1', - 'external_ids': {}, 'networks': ['20.0.1.0/24']}, + 'external_ids': { + ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY: + utils.ovn_name('lr-id-b'), + }, 'networks': ['20.0.1.0/24']}, {'name': utils.ovn_lrouter_port_name('orp-id-b2'), - 'external_ids': {}, 'networks': ['20.0.2.0/24'], + 'external_ids': { + ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY: + utils.ovn_name('lr-id-b')}, + 'networks': ['20.0.2.0/24'], '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'], + 'external_ids': { + ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY: + utils.ovn_name('lr-id-b')}, + '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', ovn_const.OVN_ROUTER_IS_EXT_GW: str(True)}, 'networks': ['10.0.4.0/24'], + 'options': {}}, + {'name': utils.ovn_lrouter_port_name('not-managed'), + 'external_ids': { + 'owner': 'not-owned-by-neutron'}, + 'networks': ['10.0.5.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'}, + 'nexthop': '10.0.3.253', + 'external_ids': { + ovn_const.OVN_SUBNET_EXT_ID_KEY: 'uuid_1'}}, {'ip_prefix': '10.0.0.0/16', - 'nexthop': '20.0.2.253'}], + 'nexthop': '20.0.2.253', + 'external_ids': { + ovn_const.OVN_SUBNET_EXT_ID_KEY: 'uuid_2'}}, + {'ip_prefix': '30.0.0.0/16', + 'nexthop': '30.0.4.253', + 'external_ids': {'owner': 'not-owned-by-neutron'}}], 'nats': [{'external_ip': '10.0.3.1', 'logical_ip': '20.0.0.0/16', 'type': 'snat'}, {'external_ip': '20.0.2.1', 'logical_ip': '10.0.0.0/24', @@ -481,7 +502,7 @@ def test_get_all_logical_switches_with_ports(self): def _test_get_all_logical_routers_with_rports(self, is_gw_port): # Test empty - mapping = self.nb_ovn_idl.get_all_logical_switches_with_ports() + mapping = self.nb_ovn_idl.get_all_logical_routers_with_rports() self.assertCountEqual(mapping, {}) # Test loaded values self._load_nb_db() diff --git a/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_maintenance.py b/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_maintenance.py index 613c6563a2d..c1028747c7e 100644 --- a/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_maintenance.py +++ b/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_maintenance.py @@ -401,30 +401,44 @@ def test_check_for_igmp_snoop_support(self): attrs={'name': 'ls0', 'other_config': { constants.MCAST_SNOOP: 'false', - constants.MCAST_FLOOD_UNREGISTERED: 'false'}}) + constants.MCAST_FLOOD_UNREGISTERED: 'false'}, + 'external_ids': { + constants.OVN_NETWORK_NAME_EXT_ID_KEY: 'port0'}}) ls1 = fakes.FakeOvsdbRow.create_one_ovsdb_row( attrs={'name': 'ls1', - 'other_config': {}}) + 'other_config': {}, + 'external_ids': { + constants.OVN_NETWORK_NAME_EXT_ID_KEY: 'port1'}}) ls2 = fakes.FakeOvsdbRow.create_one_ovsdb_row( attrs={'name': 'ls2', 'other_config': { constants.MCAST_SNOOP: 'true', - constants.MCAST_FLOOD_UNREGISTERED: 'false'}}) + constants.MCAST_FLOOD_UNREGISTERED: 'false'}, + 'external_ids': { + constants.OVN_NETWORK_NAME_EXT_ID_KEY: 'port2'}}) ls3 = fakes.FakeOvsdbRow.create_one_ovsdb_row( attrs={'name': '', - 'other_config': {}}) + 'other_config': {}, + 'external_ids': { + constants.OVN_NETWORK_NAME_EXT_ID_KEY: 'port3'}}) ls4 = fakes.FakeOvsdbRow.create_one_ovsdb_row( attrs={'name': '', - 'other_config': {constants.MCAST_SNOOP: 'false'}}) - + 'other_config': {constants.MCAST_SNOOP: 'false'}, + 'external_ids': { + constants.OVN_NETWORK_NAME_EXT_ID_KEY: 'port4'}}) + ls5 = fakes.FakeOvsdbRow.create_one_ovsdb_row( + attrs={'name': 'ls5', + 'other_config': {}, + 'external_ids': {}}) nb_idl.ls_list.return_value.execute.return_value = [ls0, ls1, ls2, ls3, - ls4] + ls4, ls5] self.assertRaises(periodics.NeverAgain, self.periodic.check_for_igmp_snoop_support) # "ls2" is not part of the transaction because it already - # have the right value set; "ls3" and "ls4" do not have a name set. + # have the right value set; "ls3" and "ls4" do not have a name set; + # "ls5" is not managed by neutron. expected_calls = [ mock.call('Logical_Switch', 'ls0', ('other_config', { @@ -479,7 +493,14 @@ def test_check_for_ha_chassis_group(self, 'external_ids': { constants.OVN_NETWORK_NAME_EXT_ID_KEY: 'neutron-net1'}}) - nb_idl.db_find_rows.return_value.execute.return_value = [p0, p1] + # Port p2 is not owned by Neutron and should not be affected. + p2 = fakes.FakeOvsdbRow.create_one_ovsdb_row( + attrs={'type': constants.LSP_TYPE_EXTERNAL, + 'name': 'p2', + 'ha_chassis_group': [hcg1], + 'external_ids': {}}) + + nb_idl.db_find_rows.return_value.execute.return_value = [p0, p1, p2] mock_sync_ha_chassis_group_network.return_value = hcg0.uuid, mock.ANY # Invoke the periodic method, it meant to run only once at startup diff --git a/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_ovn_db_sync.py b/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_ovn_db_sync.py index cf9cd85eca6..25f5812ce99 100644 --- a/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_ovn_db_sync.py +++ b/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_ovn_db_sync.py @@ -147,7 +147,8 @@ def setUp(self): 'security_group_id': 'sg2'}], 'name': 'all-tcpe'}] - self.sg_port_groups_ovn = [mock.Mock(), mock.Mock(), mock.Mock()] + self.sg_port_groups_ovn = [ + mock.Mock(), mock.Mock(), mock.Mock(), mock.Mock()] self.sg_port_groups_ovn[0].configure_mock( name='pg_sg1', external_ids={ovn_const.OVN_SG_EXT_ID_KEY: 'sg1'}, @@ -159,8 +160,13 @@ def setUp(self): ports=[], acls=[]) self.sg_port_groups_ovn[2].configure_mock( - name='neutron_pg_drop', - external_ids=[], + name=ovn_const.OVN_DROP_PORT_GROUP_NAME, + external_ids={}, + ports=[], + acls=[]) + self.sg_port_groups_ovn[3].configure_mock( + name='external_pg', + external_ids={'owner': 'not-owned-by-neutron'}, ports=[], acls=[]) From ed988ed2b6acbc98e4eca23893a05c389af97f61 Mon Sep 17 00:00:00 2001 From: Slawek Kaplonski Date: Fri, 16 Jan 2026 17:24:20 +0100 Subject: [PATCH 2/3] Don't delete from OVN DB ACLs not managed by Neutron Neutron ML2/OVN driver is using ACLs to implement Security Group rules in the OVN backend but neutron-ovn-db-sync util was trying to clean OVN DB slightly too agressively and when it was run in the "repair" mode it was removing from OVN DB all ACLs which did not had security group rule id in the external_ids. That could break e.g. ACLs created by the neutron-fwaas as its OVN driver is also using ACLs to implement rules but is adding different key in the external_ids. This patch changes logic of the neutron-ovn-db-sync-util tool so that it will skip all ACL rules which don't have in external_ids key related to the SG rule. This is similar change to what was already done e.g. for Port Groups in [1]. [1] https://review.opendev.org/c/openstack/neutron/+/958679 Closes-bug: #2138430 Conflicts: neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_ovn_db_sync.py Change-Id: I12f018d29e7d00d1f0fb0272dc905d1026633cbb Signed-off-by: Slawek Kaplonski (cherry picked from commit 7524142d7599d1ad7764a8e79a48130e8a631199) --- .../ovn/mech_driver/ovsdb/ovn_db_sync.py | 5 + .../ovn/mech_driver/ovsdb/test_ovn_db_sync.py | 261 ++++++++++++++---- ...longs-to-the-neutron-f0758ac56f8dd2d7.yaml | 9 + 3 files changed, 220 insertions(+), 55 deletions(-) create mode 100644 releasenotes/notes/do-not-sync-OVN-ACLs-which-do-not-belongs-to-the-neutron-f0758ac56f8dd2d7.yaml diff --git a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_db_sync.py b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_db_sync.py index 4901f1fda90..9d95cf8dcce 100644 --- a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_db_sync.py +++ b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_db_sync.py @@ -264,6 +264,11 @@ def _get_acls_from_port_groups(self): acl_string['port_group'] = pg.name if id_key in acl.external_ids: acl_string[id_key] = acl.external_ids[id_key] + elif pg.name != ovn_const.OVN_DROP_PORT_GROUP_NAME: + # If ACL is not associated with a security group rule, + # nor it belongs to the default neutron_pg_drop port group, + # it don't need to be synced. + continue # This properties are present as lists of one item, # converting them to string. if acl_string['name']: diff --git a/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_ovn_db_sync.py b/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_ovn_db_sync.py index 25f5812ce99..d0b97e99e59 100644 --- a/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_ovn_db_sync.py +++ b/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_ovn_db_sync.py @@ -33,6 +33,13 @@ OvnPortInfo = collections.namedtuple('OvnPortInfo', ['name']) +class FakeACL: + + def __init__(self, **kwargs): + for k, v in kwargs.items(): + setattr(self, k, v) + + @mock.patch.object(ovn_plugin.OVNL3RouterPlugin, '_sb_ovn', mock.Mock()) class TestOvnNbSyncML2(test_mech_driver.OVNMechanismDriverTestCase): @@ -121,55 +128,42 @@ def setUp(self): 'host_routes': [], 'ip_version': 4}] + self.security_group_rules = [ + { + 'remote_group_id': None, + 'direction': 'ingress', + 'remote_ip_prefix': const.IPv4_ANY, + 'protocol': 'tcp', + 'ethertype': 'IPv4', + 'project_id': 'project1', + 'port_range_max': 65535, + 'port_range_min': 1, + 'id': 'ruleid1', + 'security_group_id': 'sg1', + 'normalized_cidr': '' + }, { + 'remote_group_id': 'sg2', + 'direction': 'egress', + 'remote_ip_prefix': const.IPv4_ANY, + 'protocol': 'tcp', + 'ethertype': 'IPv4', + 'project_id': 'project1', + 'port_range_max': 65535, + 'port_range_min': 1, + 'id': 'ruleid2', + 'security_group_id': 'sg2', + 'normalized_cidr': '' + }, + ] + self.security_groups = [ - {'id': 'sg1', 'tenant_id': 'tenant1', - 'security_group_rules': [{'remote_group_id': None, - 'direction': 'ingress', - 'remote_ip_prefix': const.IPv4_ANY, - 'protocol': 'tcp', - 'ethertype': 'IPv4', - 'tenant_id': 'tenant1', - 'port_range_max': 65535, - 'port_range_min': 1, - 'id': 'ruleid1', - 'security_group_id': 'sg1'}], + {'id': 'sg1', 'project_id': 'project1', + 'security_group_rules': [self.security_group_rules[0]], 'name': 'all-tcp'}, - {'id': 'sg2', 'tenant_id': 'tenant1', - 'security_group_rules': [{'remote_group_id': 'sg2', - 'direction': 'egress', - 'remote_ip_prefix': const.IPv4_ANY, - 'protocol': 'tcp', - 'ethertype': 'IPv4', - 'tenant_id': 'tenant1', - 'port_range_max': 65535, - 'port_range_min': 1, - 'id': 'ruleid1', - 'security_group_id': 'sg2'}], + {'id': 'sg2', 'project_id': 'project1', + 'security_group_rules': [self.security_group_rules[1]], 'name': 'all-tcpe'}] - self.sg_port_groups_ovn = [ - mock.Mock(), mock.Mock(), mock.Mock(), mock.Mock()] - self.sg_port_groups_ovn[0].configure_mock( - name='pg_sg1', - external_ids={ovn_const.OVN_SG_EXT_ID_KEY: 'sg1'}, - ports=[], - acls=[]) - self.sg_port_groups_ovn[1].configure_mock( - name='pg_unknown_del', - external_ids={ovn_const.OVN_SG_EXT_ID_KEY: 'sg2'}, - ports=[], - acls=[]) - self.sg_port_groups_ovn[2].configure_mock( - name=ovn_const.OVN_DROP_PORT_GROUP_NAME, - external_ids={}, - ports=[], - acls=[]) - self.sg_port_groups_ovn[3].configure_mock( - name='external_pg', - external_ids={'owner': 'not-owned-by-neutron'}, - ports=[], - acls=[]) - self.ports = [ {'id': 'p1n1', 'device_owner': 'compute:None', @@ -226,6 +220,90 @@ def setUp(self): 'ip_address': '90.0.0.10'}], 'network_id': 'ext-net'}] + self.sg_port_groups_ovn = [mock.Mock(), mock.Mock(), mock.Mock(), + mock.Mock(), mock.Mock()] + self.sg_port_groups_ovn[0].configure_mock( + name='pg_sg1', + external_ids={ovn_const.OVN_SG_EXT_ID_KEY: 'sg1'}, + ports=[], + acls=[FakeACL( + name=[], + meter=[], + severity=[], + direction='to-lport', + action='allow-related', + log=False, + priority=1002, + match=('outport == @pg_sg1 && ip4 && tcp && ' + 'tcp.dst >= 1 && tcp.dst <= 65535'), + external_ids={ovn_const.OVN_SG_RULE_EXT_ID_KEY: 'ruleid1'} + )]) + self.sg_port_groups_ovn[1].configure_mock( + name='pg_unknown_del', + external_ids={ovn_const.OVN_SG_EXT_ID_KEY: 'sg2'}, + ports=[], + acls=[]) + self.sg_port_groups_ovn[2].configure_mock( + name=ovn_const.OVN_DROP_PORT_GROUP_NAME, + external_ids={}, + ports=[], + acls=[ + FakeACL( + name=[], + meter=[], + severity=[], + direction='to-lport', + action='drop', + log=False, + priority=1001, + match=('outport == @%s && ip' % + ovn_const.OVN_DROP_PORT_GROUP_NAME), + external_ids={} + ), + FakeACL( + name=[], + meter=[], + severity=[], + direction='from-lport', + action='drop', + log=False, + priority=1001, + match=('inport == @%s && ip' % + ovn_const.OVN_DROP_PORT_GROUP_NAME), + external_ids={} + ) + ]) + self.sg_port_groups_ovn[3].configure_mock( + name='pg_sg_stale', + external_ids={ovn_const.OVN_SG_EXT_ID_KEY: 'sg_stale'}, + ports=[], + acls=[FakeACL( + name=[], + meter=[], + severity=[], + direction='to-lport', + action='allow-related', + log=False, + priority=1000, + match='outport == @pg_sg_stale', + external_ids={ovn_const.OVN_SG_RULE_EXT_ID_KEY: 'stale_rule'} + )]) + self.sg_port_groups_ovn[4].configure_mock( + name='external_pg', + external_ids={'owner': 'not-owned-by-neutron'}, + ports=[], + acls=[FakeACL( + name=[], + meter=[], + severity=[], + direction=[], + action='allow-related', + log=False, + priority=1000, + match='outport == @external_pg', + external_ids={'owner': 'not-owned-by-neutron'} + )]) + self.ports_ovn = [OvnPortInfo('p1n1'), OvnPortInfo('p1n2'), OvnPortInfo('p2n1'), OvnPortInfo('p2n2'), OvnPortInfo('p3n1'), OvnPortInfo('p3n3')] @@ -438,10 +516,13 @@ def wrapper(*args, **kwargs): # following block is used for acl syncing unit-test # With the given set of values in the unit testing, - # 19 neutron acls should have been there, - # 4 acls are returned as current ovn acls, - # two of which will match with neutron. - # So, in this example 17 will be added, 2 removed + # 5 Port Groups are created in the OVN db, + # 2 of them should be deleted as they are managed by Neutron SGs and + # don't match any SG in the Neutron DB. + # One of those Port Groups has also ACL which should be deleted. + # One Port Group is missing (sg2) and should be created in OVN DB. + # There is one ACL in that missing ACL which should be created in the + # OVN DB too. core_plugin.get_ports = mock.Mock() core_plugin.get_ports.side_effect = get_ports() @@ -467,10 +548,45 @@ def wrapper(*args, **kwargs): ovn_nb_synchronizer.get_acls.return_value = self.acls_ovn core_plugin.get_security_groups = mock.MagicMock( return_value=self.security_groups) + + def get_security_group_rules(context, filters=None): + rules = [] + for rule in self.security_group_rules: + if filters['security_group_id'] == rule['security_group_id']: + rules.append(rule) + return rules + + core_plugin.get_security_group_rules = mock.MagicMock( + side_effect=get_security_group_rules) get_sg_port_groups = mock.MagicMock() + if test_logging: + for pg in self.sg_port_groups_ovn: + for pg_acl in pg.acls: + if pg.name == ovn_const.OVN_DROP_PORT_GROUP_NAME: + pg_acl.log = True + pg_acl.name = ['neutron-1111'] + pg_acl.severity = ['info'] + pg_acl.meter = ['acl_log_meter'] + # import remote_pdb; remote_pdb.set_trace(port=2222) get_sg_port_groups.execute.return_value = self.sg_port_groups_ovn ovn_api.db_list_rows.return_value = get_sg_port_groups ovn_api.lsp_list.execute.return_value = self.ports_ovn + + # we need to mock ACL table schema to actually be able to search for + # specific fields in the mocked ACL objects: + mock.patch.dict( + ovn_nb_synchronizer.ovn_api._tables['ACL'].columns, + {'name': mock.Mock(), + 'meter': mock.Mock(), + 'severity': mock.Mock(), + 'direction': mock.Mock(), + 'action': mock.Mock(), + 'priority': mock.Mock(), + 'match': mock.Mock(), + 'log': mock.Mock(), + 'external_ids': mock.Mock()} + ).start() + # end of acl-sync block # The following block is used for router and router port syncing tests @@ -606,6 +722,8 @@ def _test_ovn_nb_sync_helper(self, ovn_nb_synchronizer, delete_dhcp_options_list, add_port_groups_list, del_port_groups_list, + add_acls_list, + del_acls_list, create_metadata_list, test_logging=False): self._test_mocks_helper(ovn_nb_synchronizer, test_logging) @@ -631,6 +749,21 @@ def _test_ovn_nb_sync_helper(self, ovn_nb_synchronizer, ovn_api.pg_del.assert_has_calls( del_port_groups_calls, any_order=True) + add_acls_calls = [mock.call(may_exist=True, **a) + for a in add_acls_list] + self.assertEqual( + len(add_acls_list), + ovn_api.pg_acl_add.call_count) + ovn_api.pg_acl_add.assert_has_calls( + add_acls_calls, any_order=True) + del_acls_calls = [mock.call(*d) + for d in del_acls_list] + ovn_api.pg_acl_del.assert_has_calls( + del_acls_calls, any_order=True) + self.assertEqual( + len(del_acls_list), + ovn_api.pg_acl_del.call_count) + self.assertEqual( len(create_network_list), ovn_nb_synchronizer._ovn_client.create_network.call_count) @@ -792,10 +925,8 @@ def _test_ovn_nb_sync_helper(self, ovn_nb_synchronizer, delete_dhcp_options_calls, any_order=True) if test_logging: - # 2 times when doing add_logging_options_to_acls and then - # 2 times because of the add_label_related used 2 times for the - # from-port and to-port drop acls - self.assertEqual(4, ovn_nb_synchronizer.ovn_log_driver. + # 2 times when doing add_logging_options_to_acls + self.assertEqual(2, ovn_nb_synchronizer.ovn_log_driver. _pgs_from_log_obj.call_count) def _test_ovn_nb_sync_mode_repair(self, test_logging=False): @@ -894,8 +1025,22 @@ def _test_ovn_nb_sync_mode_repair(self, test_logging=False): {'external_ids': {ovn_const.OVN_SG_EXT_ID_KEY: 'sg2'}, 'name': 'pg_sg2', 'acls': []}] - del_port_groups_list = ['pg_unknown_del'] - + del_port_groups_list = ['pg_unknown_del', 'pg_sg_stale'] + add_acls_list = [ + {'port_group': 'pg_sg2', + 'priority': 1002, + 'action': 'allow-related', + 'log': False, + 'name': 'neutron-1111' if test_logging else [], + 'severity': 'info' if test_logging else [], + 'direction': 'from-lport', + 'match': ('inport == @pg_sg2 && ip4 && tcp && ' + 'tcp.dst >= 1 && tcp.dst <= 65535'), + 'meter': 'acl_log_meter' if test_logging else [], + ovn_const.OVN_SG_RULE_EXT_ID_KEY: 'ruleid2'}, + ] + del_acls_list = [ + ('pg_sg_stale', 'to-lport', 1000, 'outport == @pg_sg_stale')] add_subnet_dhcp_options_list = [(self.subnets[2], self.networks[1]), (self.subnets[1], self.networks[0])] delete_dhcp_options_list = ['UUID2', 'UUID4', 'UUID5'] @@ -926,6 +1071,8 @@ def _test_ovn_nb_sync_mode_repair(self, test_logging=False): delete_dhcp_options_list, add_port_groups_list, del_port_groups_list, + add_acls_list, + del_acls_list, create_metadata_list, test_logging) @@ -956,6 +1103,8 @@ def test_ovn_nb_sync_mode_log(self): delete_dhcp_options_list = [] add_port_groups_list = [] del_port_groups_list = [] + add_acls_list = [] + del_acls_list = [] create_metadata_list = [] ovn_nb_synchronizer = ovn_db_sync.OvnNbSynchronizer( @@ -983,6 +1132,8 @@ def test_ovn_nb_sync_mode_log(self): delete_dhcp_options_list, add_port_groups_list, del_port_groups_list, + add_acls_list, + del_acls_list, create_metadata_list) def _test_ovn_nb_sync_calculate_routes_helper(self, diff --git a/releasenotes/notes/do-not-sync-OVN-ACLs-which-do-not-belongs-to-the-neutron-f0758ac56f8dd2d7.yaml b/releasenotes/notes/do-not-sync-OVN-ACLs-which-do-not-belongs-to-the-neutron-f0758ac56f8dd2d7.yaml new file mode 100644 index 00000000000..97311cb85c8 --- /dev/null +++ b/releasenotes/notes/do-not-sync-OVN-ACLs-which-do-not-belongs-to-the-neutron-f0758ac56f8dd2d7.yaml @@ -0,0 +1,9 @@ +--- +other: + - | + The ``neutron-ovn-db-sync`` utility only manages OVN ACL rules containing + the ``neutron:security_group_rule_id`` key in external_ids. Any ACL rules + lacking this key are treated as external and will remain untouched by the + synchronization process. Consequently, these rules will not be automatically + removed, which may affect the intended behavior of the security groups + created in Neutron. From 8d32160a99cb745e94aa0a4c894e68b61614caa2 Mon Sep 17 00:00:00 2001 From: Slawek Kaplonski Date: Tue, 27 Jan 2026 16:44:25 +0100 Subject: [PATCH 3/3] Cleaning debug import of the pdb module In patch [1] there was accidentally merged commented line which imports remote_pdb. This was used only for the debugging process and should never be merged. TrivialFix [1] https://review.opendev.org/c/openstack/neutron/+/973648 Conflicts: neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_ovn_db_sync.py Change-Id: I0660d88ac12596858678ac6cf64de7ed7743d7e3 Signed-off-by: Slawek Kaplonski --- .../ml2/drivers/ovn/mech_driver/ovsdb/test_ovn_db_sync.py | 1 - 1 file changed, 1 deletion(-) diff --git a/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_ovn_db_sync.py b/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_ovn_db_sync.py index d0b97e99e59..70049699cbd 100644 --- a/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_ovn_db_sync.py +++ b/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_ovn_db_sync.py @@ -567,7 +567,6 @@ def get_security_group_rules(context, filters=None): pg_acl.name = ['neutron-1111'] pg_acl.severity = ['info'] pg_acl.meter = ['acl_log_meter'] - # import remote_pdb; remote_pdb.set_trace(port=2222) get_sg_port_groups.execute.return_value = self.sg_port_groups_ovn ovn_api.db_list_rows.return_value = get_sg_port_groups ovn_api.lsp_list.execute.return_value = self.ports_ovn