Address masking issue during auto-allocation failure

If an interface could not successfully be plugged to a router
during the provisioning of external connectivity, a failure
is triggered, and a subsequent cleanup performed.

This patch ensures that the cleanup accounts only for the
subnets that were uplinked successfully, thus fixing the
incomplete cleanup, and the masking user error that resulted
otherwise.

Closes-bug: #1545210

Change-Id: I061a1407abf6ed1a39056b1f72fd039919e2fc79

neutron/services/auto_allocate/db.py

@@ -235,9 +235,11 @@ class AutoAllocatedTopologyMixin(common_db_mixin.CommonDbMixin):
         try:
             router = self.l3_plugin.create_router(
                 context, {'router': router_args})
+            attached_subnets = []
             for subnet in subnets:
                 self.l3_plugin.add_router_interface(
                     context, router['id'], {'subnet_id': subnet['id']})
+                attached_subnets.append(subnet)
             return router
         except n_exc.BadRequest:
             LOG.error(_LE("Unable to auto allocate topology for tenant "
@@ -245,7 +247,7 @@ class AutoAllocatedTopologyMixin(common_db_mixin.CommonDbMixin):
             if router:
                 self._cleanup(context,
                     network_id=subnets[0]['network_id'],
-                    router_id=router['id'], subnets=subnets)
+                    router_id=router['id'], subnets=attached_subnets)
             raise exceptions.AutoAllocationFailure(
                 reason=_("Unable to provide external connectivity"))
 

neutron/tests/unit/services/auto_allocate/test_db.py

@@ -0,0 +1,48 @@
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#    http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
+# implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+import mock
+
+from neutron.common import exceptions as n_exc
+from neutron import context
+from neutron.services.auto_allocate import db
+from neutron.services.auto_allocate import exceptions
+from neutron.tests import base
+
+
+class AutoAllocateTestCase(base.BaseTestCase):
+
+    def setUp(self):
+        super(AutoAllocateTestCase, self).setUp()
+        self.ctx = context.get_admin_context()
+        self.mixin = db.AutoAllocatedTopologyMixin()
+        self.mixin._l3_plugin = mock.Mock()
+
+    def test__provision_external_connectivity_expected_cleanup(self):
+        """Test that the right resources are cleaned up."""
+        subnets = [
+            {'id': 'subnet_foo_1', 'network_id': 'network_foo'},
+            {'id': 'subnet_foo_2', 'network_id': 'network_foo'},
+        ]
+        with mock.patch.object(self.mixin, '_cleanup') as mock_cleanup:
+            self.mixin.l3_plugin.create_router.return_value = (
+                {'id': 'router_foo'})
+            self.mixin.l3_plugin.add_router_interface.side_effect = (
+                n_exc.BadRequest(resource='router', msg='doh!'))
+            self.assertRaises(exceptions.AutoAllocationFailure,
+                self.mixin._provision_external_connectivity,
+                self.ctx, 'ext_net_foo', subnets, 'tenant_foo')
+            # expect no subnets to be unplugged
+            mock_cleanup.assert_called_once_with(
+                self.ctx, network_id='network_foo',
+                router_id='router_foo', subnets=[])