From 96ba199d733944e5b8aa3664a04d9204fd66c878 Mon Sep 17 00:00:00 2001 From: Oleg Bondarev Date: Tue, 15 Dec 2015 17:58:51 +0300 Subject: [PATCH] Use admin context when removing DVR router on vm port deletion In case non-admin tenant removes last VM on a shared network (owned by admin) connected to a DVR router (also owned by admin) we need to remove the router from the host where there are no more dvr serviceable ports. Commit edbade486102a219810137d1c6b916e87475d477 fixed logic that determines routers that should be removed from host. However in order to actually remove the router we also need admin context. This was not caught by unit tests and one reason for that is so called 'mock everything' approach which is evil and generally useless. This patch replaces unit tests with functional tests that we able to catch the bug. Closes-Bug: #1424096 Change-Id: Ia6cdf2294562c2a2727350c78eeab155097e0c33 --- neutron/db/l3_dvrscheduler_db.py | 5 +- .../l3_router/test_l3_dvr_router_plugin.py | 51 ++++- .../unit/scheduler/test_l3_agent_scheduler.py | 197 +----------------- 3 files changed, 57 insertions(+), 196 deletions(-) diff --git a/neutron/db/l3_dvrscheduler_db.py b/neutron/db/l3_dvrscheduler_db.py index 497546725..ada1c799a 100644 --- a/neutron/db/l3_dvrscheduler_db.py +++ b/neutron/db/l3_dvrscheduler_db.py @@ -460,8 +460,11 @@ def _notify_port_delete(event, resource, trigger, **kwargs): service_constants.L3_ROUTER_NAT) l3plugin.update_arp_entry_for_dvr_service_port(context, port, "del") for router in removed_routers: + # we need admin context in case a tenant removes the last dvr + # serviceable port on a shared network owned by admin, where router + # is also owned by admin l3plugin.remove_router_from_l3_agent( - context, router['agent_id'], router['router_id']) + context.elevated(), router['agent_id'], router['router_id']) def _notify_l3_agent_port_update(resource, event, trigger, **kwargs): diff --git a/neutron/tests/functional/services/l3_router/test_l3_dvr_router_plugin.py b/neutron/tests/functional/services/l3_router/test_l3_dvr_router_plugin.py index dd2c70435..c15c42dea 100644 --- a/neutron/tests/functional/services/l3_router/test_l3_dvr_router_plugin.py +++ b/neutron/tests/functional/services/l3_router/test_l3_dvr_router_plugin.py @@ -16,6 +16,7 @@ import mock from neutron.api.v2 import attributes from neutron.common import constants +from neutron import context from neutron.extensions import external_net from neutron.extensions import portbindings from neutron.tests.common import helpers @@ -509,4 +510,52 @@ class L3DvrTestCase(ml2_test_base.ML2TestFramework): l3_notifier.routers_updated_on_host.assert_called_once_with( self.context, {router['id']}, HOST2) l3_notifier.router_removed_from_agent.assert_called_once_with( - self.context, router['id'], HOST1) + mock.ANY, router['id'], HOST1) + + def _test_router_remove_from_agent_on_vm_port_deletion( + self, non_admin_port=False): + # register l3 agent in dvr mode in addition to existing dvr_snat agent + HOST = 'host1' + non_admin_tenant = 'tenant1' + dvr_agent = helpers.register_l3_agent( + host=HOST, agent_mode=constants.L3_AGENT_MODE_DVR) + router = self._create_router() + with self.network(shared=True) as net,\ + self.subnet(network=net) as subnet,\ + self.port(subnet=subnet, + device_owner=DEVICE_OWNER_COMPUTE, + tenant_id=non_admin_tenant, + set_context=non_admin_port) as port: + self.core_plugin.update_port( + self.context, port['port']['id'], + {'port': {portbindings.HOST_ID: HOST}}) + self.l3_plugin.add_router_interface( + self.context, router['id'], + {'subnet_id': subnet['subnet']['id']}) + + # router should be scheduled to agent on HOST + agents = self.l3_plugin.list_l3_agents_hosting_router( + self.context, router['id']) + self.assertEqual(1, len(agents['agents'])) + self.assertEqual(dvr_agent['id'], agents['agents'][0]['id']) + + notifier = self.l3_plugin.agent_notifiers[ + constants.AGENT_TYPE_L3] + with mock.patch.object( + notifier, 'router_removed_from_agent') as remove_mock: + ctx = context.Context( + '', non_admin_tenant) if non_admin_port else self.context + self._delete('ports', port['port']['id'], neutron_context=ctx) + # now when port is deleted the router should be unscheduled + agents = self.l3_plugin.list_l3_agents_hosting_router( + self.context, router['id']) + self.assertEqual(0, len(agents['agents'])) + remove_mock.assert_called_once_with( + mock.ANY, router['id'], HOST) + + def test_router_remove_from_agent_on_vm_port_deletion(self): + self._test_router_remove_from_agent_on_vm_port_deletion() + + def test_admin_router_remove_from_agent_on_vm_port_deletion(self): + self._test_router_remove_from_agent_on_vm_port_deletion( + non_admin_port=True) diff --git a/neutron/tests/unit/scheduler/test_l3_agent_scheduler.py b/neutron/tests/unit/scheduler/test_l3_agent_scheduler.py index 80a5db52b..db991108a 100644 --- a/neutron/tests/unit/scheduler/test_l3_agent_scheduler.py +++ b/neutron/tests/unit/scheduler/test_l3_agent_scheduler.py @@ -1029,7 +1029,7 @@ class L3DvrSchedulerTestCase(testlib_api.SqlTestCase): l3_dvrscheduler_db._notify_l3_agent_port_update( 'port', 'after_update', mock.ANY, **kwargs) l3plugin.remove_router_from_l3_agent.assert_called_once_with( - self.adminContext, 'foo_agent', 'foo_id') + mock.ANY, 'foo_agent', 'foo_id') self.assertEqual( 2, l3plugin.update_arp_entry_for_dvr_service_port.call_count) l3plugin.dvr_handle_new_service_port.assert_called_once_with( @@ -1078,7 +1078,7 @@ class L3DvrSchedulerTestCase(testlib_api.SqlTestCase): self.assertFalse( l3plugin.dvr_handle_new_service_port.called) l3plugin.remove_router_from_l3_agent.assert_called_once_with( - self.adminContext, 'foo_agent', 'foo_id') + mock.ANY, 'foo_agent', 'foo_id') def test__notify_port_delete(self): plugin = manager.NeutronManager.get_plugin() @@ -1103,7 +1103,7 @@ class L3DvrSchedulerTestCase(testlib_api.SqlTestCase): assert_called_once_with( self.adminContext, mock.ANY, 'del') l3plugin.remove_router_from_l3_agent.assert_called_once_with( - self.adminContext, 'foo_agent', 'foo_id') + mock.ANY, 'foo_agent', 'foo_id') def test_dvr_handle_new_service_port(self): port = { @@ -1231,197 +1231,6 @@ class L3DvrSchedulerTestCase(testlib_api.SqlTestCase): r1['id']) self.assertEqual(len(sub_ids), 0) - def _create_port(self, port_name, tenant_id, host, subnet_id, ip_address, - status='ACTIVE', - device_owner=DEVICE_OWNER_COMPUTE_NOVA): - return { - 'id': port_name + '-port-id', - 'tenant_id': tenant_id, - 'device_id': port_name, - 'device_owner': device_owner, - 'status': status, - portbindings.HOST_ID: host, - 'fixed_ips': [ - { - 'subnet_id': subnet_id, - 'ip_address': ip_address - } - ] - } - - def test_dvr_deletens_if_no_port_no_routers(self): - # Delete a vm port, the port subnet has no router interface. - vm_tenant_id = 'tenant-1' - my_context = n_context.Context('user-1', vm_tenant_id, is_admin=False) - vm_port_host = 'compute-node-1' - - vm_port = self._create_port( - 'deleted-vm', vm_tenant_id, vm_port_host, - 'shared-subnet', '10.10.10.3', - status='INACTIVE') - - vm_port_id = vm_port['id'] - fakePortDB = FakePortDB([vm_port]) - - with mock.patch.object(my_context, - 'elevated', - return_value=self.adminContext),\ - mock.patch( - 'neutron.plugins.ml2.db.get_port_binding_host', - return_value=vm_port_host) as mock_get_port_binding_host,\ - mock.patch('neutron.db.db_base_plugin_v2.NeutronDbPluginV2.' - 'get_ports', side_effect=fakePortDB.get_ports),\ - mock.patch('neutron.db.db_base_plugin_v2.NeutronDbPluginV2.' - 'get_port', return_value=vm_port): - - routers = self.dut.dvr_deletens_if_no_port(my_context, vm_port_id) - self.assertEqual([], routers) - mock_get_port_binding_host.assert_called_once_with( - self.adminContext.session, vm_port_id) - - def test_dvr_deletens_if_no_ports_no_removeable_routers(self): - # A VM port is deleted, but the router can't be unscheduled from the - # compute node because there is another VM port present. - vm_tenant_id = 'tenant-1' - my_context = n_context.Context('user-1', vm_tenant_id, is_admin=False) - shared_subnet_id = '80947d4a-fbc8-484b-9f92-623a6bfcf3e0', - vm_port_host = 'compute-node-1' - - dvr_port = self._create_port( - 'dvr-router', 'admin-tenant', vm_port_host, - shared_subnet_id, '10.10.10.1', - device_owner=constants.DEVICE_OWNER_DVR_INTERFACE) - - deleted_vm_port = self._create_port( - 'deleted-vm', vm_tenant_id, vm_port_host, - shared_subnet_id, '10.10.10.3', - status='INACTIVE') - deleted_vm_port_id = deleted_vm_port['id'] - - fakePortDB = FakePortDB([deleted_vm_port, dvr_port]) - - vm_port_binding = { - 'port_id': deleted_vm_port_id, - 'host': vm_port_host - } - - with mock.patch.object(my_context, - 'elevated', - return_value=self.adminContext),\ - mock.patch( - 'neutron.plugins.ml2.db.get_port_binding_host', - return_value=vm_port_host) as mock_get_port_binding_host,\ - mock.patch('neutron.db.db_base_plugin_v2.NeutronDbPluginV2.' - 'get_port', side_effect=fakePortDB.get_port),\ - mock.patch('neutron.db.db_base_plugin_v2.NeutronDbPluginV2.' - 'get_ports', side_effect=fakePortDB.get_ports) as\ - mock_get_ports,\ - mock.patch('neutron.db.db_base_plugin_v2.NeutronDbPluginV2.' - '_get_ports_query'),\ - mock.patch('neutron.plugins.ml2.db.' - 'get_dvr_port_binding_by_host', - return_value=vm_port_binding) as\ - mock_get_dvr_port_binding_by_host,\ - mock.patch.object(self.dut, - 'check_dvr_serviceable_ports_on_host', - return_value=True): - - routers = self.dut.dvr_deletens_if_no_port( - my_context, deleted_vm_port_id) - self.assertEqual([], routers) - - mock_get_port_binding_host.assert_called_once_with( - self.adminContext.session, deleted_vm_port_id) - self.assertTrue(mock_get_ports.called) - self.assertFalse(mock_get_dvr_port_binding_by_host.called) - - def _test_dvr_deletens_if_no_ports_delete_routers(self, - vm_tenant, - router_tenant): - class FakeAgent(object): - def __init__(self, id, host, agent_type): - self.id = id - self.host = host - self.agent_type = agent_type - - my_context = n_context.Context('user-1', vm_tenant, is_admin=False) - shared_subnet_id = '80947d4a-fbc8-484b-9f92-623a6bfcf3e0', - vm_port_host = 'compute-node-1' - - router_id = 'dvr-router' - dvr_port = self._create_port( - router_id, router_tenant, vm_port_host, - shared_subnet_id, '10.10.10.1', - device_owner=constants.DEVICE_OWNER_DVR_INTERFACE) - dvr_port_id = dvr_port['id'] - - deleted_vm_port = self._create_port( - 'deleted-vm', vm_tenant, vm_port_host, - shared_subnet_id, '10.10.10.3', - status='INACTIVE') - deleted_vm_port_id = deleted_vm_port['id'] - - fakePortDB = FakePortDB([dvr_port, deleted_vm_port]) - - dvr_port_binding = { - 'port_id': dvr_port_id, 'host': vm_port_host - } - - agent_id = 'l3-agent-on-compute-node-1' - l3_agent_on_vm_host = FakeAgent(agent_id, - vm_port_host, - constants.AGENT_TYPE_L3) - - with mock.patch.object(my_context, - 'elevated', - return_value=self.adminContext),\ - mock.patch( - 'neutron.plugins.ml2.db.get_port_binding_host', - return_value=vm_port_host) as mock_get_port_binding_host,\ - mock.patch('neutron.db.db_base_plugin_v2.NeutronDbPluginV2.' - 'get_port', side_effect=fakePortDB.get_port),\ - mock.patch('neutron.db.db_base_plugin_v2.NeutronDbPluginV2.' - 'get_ports', side_effect=fakePortDB.get_ports) as\ - mock_get_ports,\ - mock.patch('neutron.plugins.ml2.db.' - 'get_dvr_port_binding_by_host', - return_value=dvr_port_binding) as\ - mock_get_dvr_port_binding_by_host,\ - mock.patch('neutron.db.agents_db.AgentDbMixin.' - '_get_agent_by_type_and_host', - return_value=l3_agent_on_vm_host),\ - mock.patch.object(self.dut, - 'check_dvr_serviceable_ports_on_host', - return_value=False): - - routers = self.dut.dvr_deletens_if_no_port( - my_context, deleted_vm_port_id) - - expected_router = { - 'router_id': router_id, - 'host': vm_port_host, - 'agent_id': agent_id - } - self.assertEqual([expected_router], routers) - - mock_get_port_binding_host.assert_called_once_with( - self.adminContext.session, deleted_vm_port_id) - self.assertTrue(mock_get_ports.called) - mock_get_dvr_port_binding_by_host.assert_called_once_with( - my_context.session, dvr_port_id, vm_port_host) - - def test_dvr_deletens_if_no_ports_delete_admin_routers(self): - # test to see whether the last VM using a router created - # by the admin will be unscheduled on the compute node - self._test_dvr_deletens_if_no_ports_delete_routers( - 'tenant-1', 'admin-tenant') - - def test_dvr_deletens_if_no_ports_delete_tenant_routers(self): - # test to see whether the last VM using a tenant's private - # router will be unscheduled on the compute node - self._test_dvr_deletens_if_no_ports_delete_routers( - 'tenant-1', 'tenant-1') - def _prepare_schedule_snat_tests(self): agent = agents_db.Agent() agent.admin_state_up = True -- 2.45.2