Make floatingip-ip-delete atomic with neutron

The infra guys were noticing an issue where they were leaking floating ip
addresses. One of the reasons this would occur for them is they called
nova floating-ip-delete which first disassocates the floating-ip in neutron
and then deletes it. Because nova-api makes two calls to neutron if the
first one succeeds and the second fails it results in the instance no longer
being associated with the floatingip. They have retry logic but they base it on
the instance and when they go to retry cleaning up the instance the floatingip
is no longer on the instance so they never delete it.

This patch fixes this issue by creating a new method
disassocate_and_delete_floating_ip which contains the different logic
needed to atomically do this for neutron rather than putting the logic
in the api layer to do something differently if one is using neutron.

This patch also adds additional code coverage for nova-network in the
delete floatingip case.

Closes-bug: 1356157

Conflicts:
	nova/api/openstack/compute/contrib/floating_ips.py
	nova/network/base_api.py

Change-Id: I53b0c9d949404288e8687b304361a74b53d69ef3
(cherry picked from commit 5f391087d1d75dd4e7532252c312256e9c3ea2b9)

5 files changed, 110 insertions, 11 deletions

diff --git a/nova/api/openstack/compute/contrib/floating_ips.py b/nova/api/openstack/compute/contrib/floating_ips.py
index 92c42a5320..542f205699 100644
--- a/nova/api/openstack/compute/contrib/floating_ips.py
+++ b/nova/api/openstack/compute/contrib/floating_ips.py
@@ -181,16 +181,14 @@ class FloatingIPController(object):
 
         # get the associated instance object (if any)
         instance = get_instance_by_floating_ip_addr(self, context, address)
-
-        # disassociate if associated
-        if floating_ip.get('fixed_ip_id'):
-            try:
-                disassociate_floating_ip(self, context, instance, address)
-            except exception.FloatingIpNotAssociated:
-                LOG.info(_("Floating ip %s has been disassociated") % address)
-
-        # release ip from project
-        self.network_api.release_floating_ip(context, address)
+        try:
+            self.network_api.disassociate_and_release_floating_ip(
+                context, instance, floating_ip)
+        except exception.Forbidden:
+            raise webob.exc.HTTPForbidden()
+        except exception.CannotDisassociateAutoAssignedFloatingIP:
+            msg = _('Cannot disassociate auto assigned floating ip')
+            raise webob.exc.HTTPForbidden(explanation=msg)
         return webob.Response(status_int=202)
 
     def _get_ip_by_id(self, context, value):
diff --git a/nova/network/api.py b/nova/network/api.py
index 91fd3bd0ce..17d8c13154 100644
--- a/nova/network/api.py
+++ b/nova/network/api.py
@@ -226,6 +226,26 @@ class API(base.Base):
         return self.floating_manager.deallocate_floating_ip(context, address,
                  affect_auto_assigned)
 
+    def disassociate_and_release_floating_ip(self, context, instance,
+                                           floating_ip):
+        """Removes (deallocates) and deletes the floating ip.
+
+        This api call was added to allow this to be done in one operation
+        if using neutron.
+        """
+
+        address = floating_ip['address']
+        if floating_ip.get('fixed_ip_id'):
+            try:
+                self.disassociate_floating_ip(context, instance, address)
+            except exception.FloatingIpNotAssociated:
+                msg = ("Floating ip %s has already been disassociated, "
+                       "perhaps by another concurrent action.") % address
+                LOG.debug(msg)
+
+        # release ip from project
+        return self.release_floating_ip(context, address)
+
     @wrap_check_policy
     @refresh_cache
     def associate_floating_ip(self, context, instance,
diff --git a/nova/network/neutronv2/api.py b/nova/network/neutronv2/api.py
index f96167040c..b169ceb9fa 100644
--- a/nova/network/neutronv2/api.py
+++ b/nova/network/neutronv2/api.py
@@ -891,9 +891,24 @@ class API(base.Base):
         # since it is not used anywhere in nova code and I could
         # find why this parameter exists.
 
+        self._release_floating_ip(context, address)
+
+    def disassociate_and_release_floating_ip(self, context, instance,
+                                           floating_ip):
+        """Removes (deallocates) and deletes the floating ip.
+
+        This api call was added to allow this to be done in one operation
+        if using neutron.
+        """
+        self._release_floating_ip(context, floating_ip['address'],
+                                  raise_if_associated=False)
+
+    def _release_floating_ip(self, context, address,
+                             raise_if_associated=True):
         client = neutronv2.get_client(context)
         fip = self._get_floating_ip_by_address(client, address)
-        if fip['port_id']:
+
+        if raise_if_associated and fip['port_id']:
             raise exception.FloatingIpAssociated(address=address)
         client.delete_floatingip(fip['id'])
 
diff --git a/nova/tests/api/openstack/compute/contrib/test_floating_ips.py b/nova/tests/api/openstack/compute/contrib/test_floating_ips.py
index a781c6bf2b..7e2feee892 100644
--- a/nova/tests/api/openstack/compute/contrib/test_floating_ips.py
+++ b/nova/tests/api/openstack/compute/contrib/test_floating_ips.py
@@ -14,9 +14,11 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
+import contextlib
 import uuid
 
 from lxml import etree
+import mock
 import webob
 
 from nova.api.openstack.compute.contrib import floating_ips
@@ -101,6 +103,38 @@ def get_instance_by_floating_ip_addr(self, context, address):
     return None
 
 
+class FloatingIpTestNeutron(test.NoDBTestCase):
+
+    def setUp(self):
+        super(FloatingIpTestNeutron, self).setUp()
+        self.flags(network_api_class='nova.network.neutronv2.api.API')
+        self.controller = floating_ips.FloatingIPController()
+
+    def test_floatingip_delete(self):
+        req = fakes.HTTPRequest.blank('/v2/fake/os-floating-ips/1')
+        fip_val = {'address': '1.1.1.1', 'fixed_ip_id': '192.168.1.2'}
+        with contextlib.nested(
+            mock.patch.object(self.controller.network_api,
+                              'disassociate_floating_ip'),
+            mock.patch.object(self.controller.network_api,
+                              'disassociate_and_release_floating_ip'),
+            mock.patch.object(self.controller.network_api,
+                              'release_floating_ip'),
+            mock.patch.object(self.controller.network_api,
+                             'get_instance_id_by_floating_address',
+                              return_value=None),
+            mock.patch.object(self.controller.network_api,
+                              'get_floating_ip',
+                              return_value=fip_val)) as (
+                disoc_fip, dis_and_del, rel_fip, _, _):
+            self.controller.delete(req, 1)
+            self.assertFalse(disoc_fip.called)
+            self.assertFalse(rel_fip.called)
+            # Only disassociate_and_release_floating_ip is
+            # called if using neutron
+            self.assertTrue(dis_and_del.called)
+
+
 class FloatingIpTest(test.TestCase):
     floating_ip = "10.10.10.10"
     floating_ip_2 = "10.10.10.11"
@@ -163,6 +197,25 @@ class FloatingIpTest(test.TestCase):
         self._delete_floating_ip()
         super(FloatingIpTest, self).tearDown()
 
+    def test_floatingip_delete(self):
+        req = fakes.HTTPRequest.blank('/v2/fake/os-floating-ips/1')
+        fip_val = {'address': '1.1.1.1', 'fixed_ip_id': '192.168.1.2'}
+        with contextlib.nested(
+            mock.patch.object(self.controller.network_api,
+                              'disassociate_floating_ip'),
+            mock.patch.object(self.controller.network_api,
+                              'release_floating_ip'),
+            mock.patch.object(self.controller.network_api,
+                             'get_instance_id_by_floating_address',
+                              return_value=None),
+            mock.patch.object(self.controller.network_api,
+                              'get_floating_ip',
+                              return_value=fip_val)) as (
+                disoc_fip, rel_fip, _, _):
+            self.controller.delete(req, 1)
+            self.assertTrue(disoc_fip.called)
+            self.assertTrue(rel_fip.called)
+
     def test_translate_floating_ip_view(self):
         floating_ip_address = self.floating_ip
         floating_ip = db.floating_ip_get_by_address(self.context,
diff --git a/nova/tests/network/test_neutronv2.py b/nova/tests/network/test_neutronv2.py
index d3d9531397..331ae68a04 100644
--- a/nova/tests/network/test_neutronv2.py
+++ b/nova/tests/network/test_neutronv2.py
@@ -1436,6 +1436,19 @@ class TestNeutronv2(TestNeutronv2Base):
         self.mox.ReplayAll()
         api.release_floating_ip(self.context, address)
 
+    def test_disassociate_and_release_floating_ip(self):
+        api = neutronapi.API()
+        address = self.fip_unassociated['floating_ip_address']
+        fip_id = self.fip_unassociated['id']
+        floating_ip = {'address': address}
+
+        self.moxed_client.list_floatingips(floating_ip_address=address).\
+            AndReturn({'floatingips': [self.fip_unassociated]})
+        self.moxed_client.delete_floatingip(fip_id)
+        self.mox.ReplayAll()
+        api.disassociate_and_release_floating_ip(self.context, None,
+                                               floating_ip)
+
     def test_release_floating_ip_associated(self):
         api = neutronapi.API()
         address = self.fip_associated['floating_ip_address']