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

@@ -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

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)

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}

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}])