From 3785dc31a465e0ac9d5de23339daddfbc346ef7c Mon Sep 17 00:00:00 2001
From: Armando Migliaccio <armamig@gmail.com>
Date: Fri, 12 Feb 2016 16:46:13 -0800
Subject: [PATCH] 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          |  4 +-
 .../unit/services/auto_allocate/__init__.py   |  0
 .../unit/services/auto_allocate/test_db.py    | 48 +++++++++++++++++++
 3 files changed, 51 insertions(+), 1 deletion(-)
 create mode 100644 neutron/tests/unit/services/auto_allocate/__init__.py
 create mode 100644 neutron/tests/unit/services/auto_allocate/test_db.py

diff --git a/neutron/services/auto_allocate/db.py b/neutron/services/auto_allocate/db.py
index b95338941b0..d3c29f83dbf 100644
--- a/neutron/services/auto_allocate/db.py
+++ b/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"))
 
diff --git a/neutron/tests/unit/services/auto_allocate/__init__.py b/neutron/tests/unit/services/auto_allocate/__init__.py
new file mode 100644
index 00000000000..e69de29bb2d
diff --git a/neutron/tests/unit/services/auto_allocate/test_db.py b/neutron/tests/unit/services/auto_allocate/test_db.py
new file mode 100644
index 00000000000..10e663038f5
--- /dev/null
+++ b/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=[])