From dfa702fac80312b0c5b93c78e92f9d01ea11bbd6 Mon Sep 17 00:00:00 2001 From: Armando Migliaccio Date: Fri, 19 Aug 2016 00:18:54 -0700 Subject: [PATCH] Implement the DELETE method for get-me-a-network Review [1] showed how tricky it can be to let the client side deal with auto-network-topology cleanups. Rather than pushing this complexity to the client, we should implement the DELETE method for this extension, as it's rather trival to do on this server side. Since the DELETE method is exposed, but it fails with 500, it is reasonable to deal with this as a bug fix, rather than having to go through yet another extension. The neutronclient side support should be added, but since the first user of this is Tempest, we can safely assume they can leverage this directly without depending on a python-neutronclient version bump. [1] https://review.openstack.org/#/c/327191/ Closes-bug: #1614872 Change-Id: I2fba51bdf8c781fcc0449e1e9947de976c96eec4 --- neutron/services/auto_allocate/db.py | 55 +++++++++++++------ .../api/test_auto_allocated_topology.py | 20 ++++++- .../services/network/json/network_client.py | 6 ++ .../unit/services/auto_allocate/test_db.py | 15 +++++ 4 files changed, 75 insertions(+), 21 deletions(-) diff --git a/neutron/services/auto_allocate/db.py b/neutron/services/auto_allocate/db.py index 513859d0aa2..5677ae9def1 100644 --- a/neutron/services/auto_allocate/db.py +++ b/neutron/services/auto_allocate/db.py @@ -114,6 +114,7 @@ class AutoAllocatedTopologyMixin(common_db_mixin.CommonDbMixin): The topology will be provisioned upon return, if network is missing. """ + fields = fields or [] tenant_id = self._validate(context, tenant_id) if CHECK_REQUIREMENTS in fields: # for dry-run requests, simply validates that subsequent @@ -138,6 +139,17 @@ class AutoAllocatedTopologyMixin(common_db_mixin.CommonDbMixin): context, tenant_id, default_external_network) return self._response(network_id, tenant_id, fields=fields) + def delete_auto_allocated_topology(self, context, tenant_id): + tenant_id = self._validate(context, tenant_id) + topology = self._get_auto_allocated_topology(context, tenant_id) + if topology: + subnets = self.core_plugin.get_subnets( + context, + filters={'network_id': [topology['network_id']]}) + self._cleanup( + context, network_id=topology['network_id'], + router_id=topology['router_id'], subnets=subnets) + def _build_topology(self, context, tenant_id, default_external_network): """Build the network topology and returns its network UUID.""" network_id = None @@ -187,12 +199,15 @@ class AutoAllocatedTopologyMixin(common_db_mixin.CommonDbMixin): return tenant_id - def _get_auto_allocated_network(self, context, tenant_id): - """Get the auto allocated network for the tenant.""" + def _get_auto_allocated_topology(self, context, tenant_id): + """Return the auto allocated topology record if present or None.""" with context.session.begin(subtransactions=True): - network = (context.session.query(models.AutoAllocatedTopology). + return (context.session.query(models.AutoAllocatedTopology). filter_by(tenant_id=tenant_id).first()) + def _get_auto_allocated_network(self, context, tenant_id): + """Get the auto allocated network for the tenant.""" + network = self._get_auto_allocated_topology(context, tenant_id) if network: return network['network_id'] @@ -333,25 +348,29 @@ class AutoAllocatedTopologyMixin(common_db_mixin.CommonDbMixin): network_id = self._get_auto_allocated_network(context, tenant_id) return network_id + # FIXME(kevinbenton): get rid of the retry once bug/1612798 is resolved @db_api.retry_db_errors def _cleanup(self, context, network_id=None, router_id=None, subnets=None): """Clean up auto allocated resources.""" - # TODO(kevinbenton): get rid of the retry and notfound exception - # handlers once bug/1612798 is resolved + # Concurrent attempts to delete the topology may interleave and + # cause some operations to fail with NotFound exceptions. Rather + # than fail partially, the exceptions should be ignored and the + # cleanup should proceed uninterrupted. if router_id: for subnet in subnets or []: - try: - self.l3_plugin.remove_router_interface( - context, router_id, {'subnet_id': subnet['id']}) - except n_exc.NotFound: - pass - try: - self.l3_plugin.delete_router(context, router_id) - except n_exc.NotFound: - pass + ignore_notfound( + self.l3_plugin.remove_router_interface, + context, router_id, {'subnet_id': subnet['id']}) + ignore_notfound(self.l3_plugin.delete_router, context, router_id) if network_id: - try: - self.core_plugin.delete_network(context, network_id) - except n_exc.NotFound: - pass + ignore_notfound( + self.core_plugin.delete_network, context, network_id) + + +def ignore_notfound(func, *args, **kwargs): + """Call the given function and pass if a `NotFound` exception is raised.""" + try: + return func(*args, **kwargs) + except n_exc.NotFound: + pass diff --git a/neutron/tests/tempest/api/test_auto_allocated_topology.py b/neutron/tests/tempest/api/test_auto_allocated_topology.py index afdfe8c941f..65c30572511 100644 --- a/neutron/tests/tempest/api/test_auto_allocated_topology.py +++ b/neutron/tests/tempest/api/test_auto_allocated_topology.py @@ -22,11 +22,14 @@ from neutron.tests.tempest.api import base class TestAutoAllocatedTopology(base.BaseAdminNetworkTest): """ - NOTE: This test may eventually migrate to Tempest. - - Tests the Get-Me-A-Network operation in the Neutron API + Tests the Get-Me-A-Network operations in the Neutron API using the REST client for Neutron. """ + # NOTE(armax): this is a precaution to avoid interference + # from other tests exercising this extension. So long as + # all tests are added under TestAutoAllocatedTopology, + # nothing bad should happen. + force_tenant_isolation = True @classmethod @test.requires_ext(extension="auto-allocated-topology", service="network") @@ -101,3 +104,14 @@ class TestAutoAllocatedTopology(base.BaseAdminNetworkTest): # After the initial GET, the API should be idempotent self.assertEqual(network_id1, network_id2) self.assertEqual(resources_after1, resources_after2) + + @test.idempotent_id('aabc0b02-cee4-11e5-9f3c-091127605a2b') + def test_delete_allocated_net_topology_as_tenant(self): + resources_before = self._count_topology_resources() + self.assertEqual((0, 0, 0), resources_before) + body = self.client.get_auto_allocated_topology() + topology = body['auto_allocated_topology'] + self.assertIsNotNone(topology) + self.client.delete_auto_allocated_topology() + resources_after = self._count_topology_resources() + self.assertEqual((0, 0, 0), resources_after) diff --git a/neutron/tests/tempest/services/network/json/network_client.py b/neutron/tests/tempest/services/network/json/network_client.py index 9c8cb056519..54a1fc3db8d 100644 --- a/neutron/tests/tempest/services/network/json/network_client.py +++ b/neutron/tests/tempest/services/network/json/network_client.py @@ -738,6 +738,12 @@ class NetworkClientJSON(service_client.RestClient): body = jsonutils.loads(body) return service_client.ResponseBody(resp, body) + def delete_auto_allocated_topology(self, tenant_id=None): + uri = '%s/auto-allocated-topology/%s' % (self.uri_prefix, tenant_id) + resp, body = self.delete(uri) + self.expected_success(204, resp.status) + return service_client.ResponseBody(resp, body) + def create_security_group_rule(self, direction, security_group_id, **kwargs): post_body = {'security_group_rule': kwargs} diff --git a/neutron/tests/unit/services/auto_allocate/test_db.py b/neutron/tests/unit/services/auto_allocate/test_db.py index ecf94a4f9af..86479f63cc9 100644 --- a/neutron/tests/unit/services/auto_allocate/test_db.py +++ b/neutron/tests/unit/services/auto_allocate/test_db.py @@ -28,6 +28,7 @@ class AutoAllocateTestCase(testlib_api.SqlTestCaseLight): self.ctx = context.get_admin_context() self.mixin = db.AutoAllocatedTopologyMixin() self.mixin._l3_plugin = mock.Mock() + self.mixin._core_plugin = mock.Mock() def test__provision_external_connectivity_expected_cleanup(self): """Test that the right resources are cleaned up.""" @@ -96,3 +97,17 @@ class AutoAllocateTestCase(testlib_api.SqlTestCaseLight): result = self.mixin._check_requirements(self.ctx, 'foo_tenant') expected = {'id': 'dry-run=pass', 'tenant_id': 'foo_tenant'} self.assertEqual(expected, result) + + def test__cleanup_handles_failures(self): + retry_then_notfound = ( + [db_exc.RetryRequest(ValueError())] + + [n_exc.NotFound()] * 10 + ) + self.mixin._l3_plugin.remove_router_interface.side_effect = ( + retry_then_notfound) + self.mixin._l3_plugin.delete_router.side_effect = ( + retry_then_notfound) + self.mixin._core_plugin.delete_network.side_effect = ( + retry_then_notfound) + self.mixin._cleanup(self.ctx, network_id=44, router_id=45, + subnets=[{'id': 46}])