From 75eefdbc69e40f9d73211852cc729bb91bb73693 Mon Sep 17 00:00:00 2001 From: Slawek Kaplonski Date: Wed, 22 May 2024 15:28:05 +0200 Subject: [PATCH 1/3] Return both project_id when validating auto allocate network When neutron API is called to check requirements for the auto_allocate topology, it needs to return not only 'tenant_id' field but also 'project_id' as that is required for the policy enforcement. Without this 'project_id' field requirements check was failing for member and reader users as they got 404 from the Neutron API. And the reason why Neutron was returning 404 was that it wasn't passing policy enforcement due to missing project_id field in the 'target' object. Closes-bug: #2066369 Change-Id: Idf96a82bc6c8cb0b47dfde3baba94b42a8a8beba (cherry picked from commit dfc01beab22f1c2b977d3e399c3fcda69a72082d) --- neutron/services/auto_allocate/db.py | 4 +++- neutron/tests/unit/services/auto_allocate/test_db.py | 5 ++++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/neutron/services/auto_allocate/db.py b/neutron/services/auto_allocate/db.py index c51777f1473..1d6d77519e8 100644 --- a/neutron/services/auto_allocate/db.py +++ b/neutron/services/auto_allocate/db.py @@ -194,7 +194,9 @@ def _check_requirements(self, context, tenant_id): except n_exc.NotFound: raise exceptions.AutoAllocationFailure( reason=_("No default subnetpools defined")) - return {'id': 'dry-run=pass', 'tenant_id': tenant_id} + return {'id': 'dry-run=pass', + 'tenant_id': tenant_id, + 'project_id': tenant_id} def _validate(self, context, tenant_id): """Validate and return the tenant to be associated to the topology.""" diff --git a/neutron/tests/unit/services/auto_allocate/test_db.py b/neutron/tests/unit/services/auto_allocate/test_db.py index 83167d7ee62..2ff3cf7955a 100644 --- a/neutron/tests/unit/services/auto_allocate/test_db.py +++ b/neutron/tests/unit/services/auto_allocate/test_db.py @@ -351,7 +351,10 @@ def test__check_requirements_happy_path_for_kevin(self): mock.patch.object( self.mixin, '_get_supported_subnetpools'): result = self.mixin._check_requirements(self.ctx, 'foo_tenant') - expected = {'id': 'dry-run=pass', 'tenant_id': 'foo_tenant'} + expected = { + 'id': 'dry-run=pass', + 'tenant_id': 'foo_tenant', + 'project_id': 'foo_tenant'} self.assertEqual(expected, result) def test__cleanup_handles_failures(self): From da8f6ef4dd2172a203f261fff535bd3f372e1276 Mon Sep 17 00:00:00 2001 From: yatinkarel Date: Tue, 28 May 2024 13:11:58 +0530 Subject: [PATCH 2/3] [stable only] Do not fail on missing logical router ports set_gateway_mtu runs for all the gateway ports for a network and if one of the ports get's deleted in meanwhile whole transaction fails. To handle this we need to add if_exists=True to the transaction but for that it needs to be supported in ovsdbapp. It's fixed in ovsdbapp with [1] but would require to bump ovsdbapp minimal version in requirements.txt which we normally don't do for stable branches. So using "update_lrouter_port" instead as that have the required option available. Before [2] that was only used but during the switch if_exists part was missed. [1] https://review.opendev.org/q/I56685478214aae7b6d3a2a3187297ad4eb1869a3 [2] https://review.opendev.org/c/openstack/neutron/+/762695 Closes-Bug: #2065701 Related-Bug: #2060163 Change-Id: I447990509cdea9830228d3bc92a97062cc57a472 (cherry picked from commit 5bdd0efb3970a52c60043f166bc728778ac3f395) Conflicts: neutron/tests/unit/fake_resources.py --- .../plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_client.py | 3 ++- neutron/tests/unit/fake_resources.py | 1 - .../plugins/ml2/drivers/ovn/mech_driver/test_mech_driver.py | 4 ++-- neutron/tests/unit/services/ovn_l3/test_plugin.py | 2 +- 4 files changed, 5 insertions(+), 5 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 97e5a9488af..f5080b35381 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 @@ -1983,7 +1983,8 @@ def set_gateway_mtu(self, context, prov_net, txn=None): for port in ports: lrp_name = utils.ovn_lrouter_port_name(port['id']) options = self._gen_router_port_options(port, prov_net) - commands.append(self._nb_idl.lrp_set_options(lrp_name, **options)) + commands.append(self._nb_idl.update_lrouter_port( + lrp_name, if_exists=True, **options)) self._transaction(commands, txn=txn) def _check_network_changes_in_ha_chassis_groups(self, context, lswitch, diff --git a/neutron/tests/unit/fake_resources.py b/neutron/tests/unit/fake_resources.py index d6fd6ada876..ebbb374d73c 100644 --- a/neutron/tests/unit/fake_resources.py +++ b/neutron/tests/unit/fake_resources.py @@ -62,7 +62,6 @@ def __init__(self, **kwargs): self.get_acls_for_lswitches = mock.Mock() self.create_lrouter = mock.Mock() self.lrp_del = mock.Mock() - self.lrp_set_options = mock.Mock() self.update_lrouter = mock.Mock() self.delete_lrouter = mock.Mock() self.add_lrouter_port = mock.Mock() 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 f0f98af7f05..b7e3ff22556 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 @@ -2484,8 +2484,8 @@ def _test_update_network_fragmentation(self, new_mtu, expected_opts, grps): self.mech_driver.update_network_postcommit(fake_ctx) lrp_name = ovn_utils.ovn_lrouter_port_name(port['port']['id']) - self.nb_ovn.lrp_set_options.assert_called_once_with( - lrp_name, **expected_opts) + self.nb_ovn.update_lrouter_port.assert_called_once_with( + lrp_name, if_exists=True, **expected_opts) def test_update_network_need_to_frag_enabled(self): ovn_conf.cfg.CONF.set_override('ovn_emit_need_to_frag', True, diff --git a/neutron/tests/unit/services/ovn_l3/test_plugin.py b/neutron/tests/unit/services/ovn_l3/test_plugin.py index b2f0a9510de..c795db28037 100644 --- a/neutron/tests/unit/services/ovn_l3/test_plugin.py +++ b/neutron/tests/unit/services/ovn_l3/test_plugin.py @@ -1689,7 +1689,7 @@ def test_add_router_interface_need_to_frag_enabled_then_remove( self.l3_inst._nb_ovn.add_lrouter_port.assert_called_once_with( **fake_router_port_assert) # Since if_exists = True it will safely return - self.l3_inst._nb_ovn.lrp_set_options( + self.l3_inst._nb_ovn.update_lrouter_port( name='lrp-router-port-id', if_exists=True, options=fake_router_port_assert) # If no if_exists is provided, it is defaulted to true, so this From fde31034aef75d08d46db0f2bb410ae01d76b9d1 Mon Sep 17 00:00:00 2001 From: yatinkarel Date: Tue, 21 May 2024 18:36:39 +0530 Subject: [PATCH 3/3] [functional tests] compatibility with ovsdbapp>=2.2.2 ovsdbapp>=2.2.2 handles cleanup of Chassis_Private record with chassis delete so we don't need explicit delete. The compatibility part can be dropped when we update requirements.txt to ovsdbapp>=2.2.2. Closes-Bug: #2066263 Change-Id: I45c6e6a1c3536cf4f2d90b01a3577eec9eaf3743 (cherry picked from commit 20b9893e34dda0b448ac75c795867cb46de5e127) (cherry picked from commit 4221f706ce269c06e2acb194612c5241dd2c1e83) (cherry picked from commit d1c5eac0a8910ded6042422c962ad20ca66b80d8) Conflicts: neutron/tests/functional/base.py --- neutron/tests/functional/base.py | 14 +++++++++++--- .../drivers/ovn/mech_driver/test_mech_driver.py | 3 ++- 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/neutron/tests/functional/base.py b/neutron/tests/functional/base.py index ca39864a6c1..bd515254c78 100644 --- a/neutron/tests/functional/base.py +++ b/neutron/tests/functional/base.py @@ -31,6 +31,7 @@ from oslo_log import log from oslo_utils import timeutils from oslo_utils import uuidutils +from ovsdbapp.backend.ovs_idl import idlutils from neutron.agent.linux import utils from neutron.api import extensions as exts @@ -473,6 +474,13 @@ def append_cms_options(ext_ids, value): def del_fake_chassis(self, chassis, if_exists=True): self.sb_api.chassis_del( chassis, if_exists=if_exists).execute(check_error=True) - if self.sb_api.is_table_present('Chassis_Private'): - self.sb_api.db_destroy( - 'Chassis_Private', chassis).execute(check_error=True) + try: + if self.sb_api.is_table_present('Chassis_Private'): + self.sb_api.db_destroy( + 'Chassis_Private', chassis).execute(check_error=True) + except idlutils.RowNotFound: + # NOTE(ykarel ): ovsdbapp >= 2.2.2 handles Chassis_Private + # record delete with chassis + # try/except can be dropped when neutron requirements.txt + # include ovsdbapp>=2.2.2 + pass diff --git a/neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/test_mech_driver.py b/neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/test_mech_driver.py index e1c63d7fce1..0ee1257acbd 100644 --- a/neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/test_mech_driver.py +++ b/neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/test_mech_driver.py @@ -1215,7 +1215,8 @@ def test_agent_list(self): # then Chassis_Private.chassis = []; both metadata and controller # agents will still be present in the agent list. agent_event = AgentWaitEvent(self.mech_driver, [self.chassis], - events=(event.RowEvent.ROW_UPDATE,)) + events=(event.RowEvent.ROW_UPDATE, + event.RowEvent.ROW_DELETE,)) self.sb_api.idl.notify_handler.watch_event(agent_event) self.sb_api.chassis_del(self.chassis).execute(check_error=True) self.assertTrue(agent_event.wait())