Rollback port after failed to add it to router

After failed to add port to a router, we cannot re-use and/or delete
this port.

With concurrent requests occuring, neutron will accept one request
and the other will be rejected with an 'overlapped CIDR' message.
Patch [1] fixed the race condition, but neutron raises
'Port already has an attached device' instead of
'overlapped CIDR', because neutron didn't cleanup the port when
the request was retried.
[1] https://review.openstack.org/#/c/303966/

This patch is needed to fix the bug completely. We will catch any
exception when adding an interface by port to a router. After that,
we rollback this port to its original state.

Change-Id: Ib68aee164a3062648fc882012d57b5e381f52196
Closes-Bug: #1535549

neutron/db/l3_db.py

@@ -702,18 +702,11 @@ class L3_NAT_dbonly_mixin(l3.RouterPluginBase,
         # Update owner before actual process in order to avoid the
         # case where a port might get attached to a router without the
         # owner successfully updating due to an unavailable backend.
-        port = self._check_router_port(context, port_id, '')
-        prev_owner = port['device_owner']
         self._core_plugin.update_port(
             context, port_id, {'port': {'device_id': router.id,
                                         'device_owner': owner}})
-        try:
-            return self._validate_router_port_info(context, router, port_id)
-        except Exception:
-            with excutils.save_and_reraise_exception():
-                self._core_plugin.update_port(
-                    context, port_id, {'port': {'device_id': '',
-                                                'device_owner': prev_owner}})
+
+        return self._validate_router_port_info(context, router, port_id)
 
     def _port_has_ipv6_address(self, port):
         for fixed_ip in port['fixed_ips']:
@@ -787,8 +780,14 @@ class L3_NAT_dbonly_mixin(l3.RouterPluginBase,
         cleanup_port = False
 
         if add_by_port:
-            port, subnets = self._add_interface_by_port(
-                    context, router, interface_info['port_id'], device_owner)
+            port_id = interface_info['port_id']
+            port = self._check_router_port(context, port_id, '')
+            revert_value = {'device_id': '',
+                            'device_owner': port['device_owner']}
+            with p_utils.update_port_on_error(
+                    self._core_plugin, context, port_id, revert_value):
+                port, subnets = self._add_interface_by_port(
+                    context, router, port_id, device_owner)
         # add_by_subnet is not used here, because the validation logic of
         # _validate_interface_info ensures that either of add_by_* is True.
         else:
@@ -796,10 +795,17 @@ class L3_NAT_dbonly_mixin(l3.RouterPluginBase,
                     context, router, interface_info['subnet_id'], device_owner)
             cleanup_port = new_port  # only cleanup port we created
 
+        if cleanup_port:
+            mgr = p_utils.delete_port_on_error(
+                self._core_plugin, context, port['id'])
+        else:
+            revert_value = {'device_id': '',
+                            'device_owner': port['device_owner']}
+            mgr = p_utils.update_port_on_error(
+                self._core_plugin, context, port['id'], revert_value)
+
         if new_port:
-            with p_utils.delete_port_on_error(self._core_plugin,
-                                              context, port['id']) as delmgr:
-                delmgr.delete_on_error = cleanup_port
+            with mgr:
                 with context.session.begin(subtransactions=True):
                     router_port = RouterPort(
                         port_id=port['id'],

neutron/db/l3_dvr_db.py

@@ -315,8 +315,14 @@ class L3_NAT_with_dvr_db_mixin(l3_db.L3_NAT_db_mixin,
         cleanup_port = False
 
         if add_by_port:
-            port, subnets = self._add_interface_by_port(
-                    context, router, interface_info['port_id'], device_owner)
+            port_id = interface_info['port_id']
+            port = self._check_router_port(context, port_id, '')
+            revert_value = {'device_id': '',
+                            'device_owner': port['device_owner']}
+            with p_utils.update_port_on_error(
+                    self._core_plugin, context, port_id, revert_value):
+                port, subnets = self._add_interface_by_port(
+                    context, router, port_id, device_owner)
         elif add_by_sub:
             port, subnets, new_port = self._add_interface_by_subnet(
                     context, router, interface_info['subnet_id'], device_owner)
@@ -324,10 +330,17 @@ class L3_NAT_with_dvr_db_mixin(l3_db.L3_NAT_db_mixin,
 
         subnet = subnets[0]
 
+        if cleanup_port:
+            mgr = p_utils.delete_port_on_error(
+                self._core_plugin, context, port['id'])
+        else:
+            revert_value = {'device_id': '',
+                            'device_owner': port['device_owner']}
+            mgr = p_utils.update_port_on_error(
+                self._core_plugin, context, port['id'], revert_value)
+
         if new_port:
-            with p_utils.delete_port_on_error(self._core_plugin,
-                                              context, port['id']) as delmgr:
-                delmgr.delete_on_error = cleanup_port
+            with mgr:
                 if router.extra_attributes.distributed and router.gw_port:
                     admin_context = context.elevated()
                     self._add_csnat_router_interface_port(

neutron/plugins/common/utils.py

@@ -197,24 +197,30 @@ def create_port(core_plugin, context, port, check_allow_post=True):
     return core_plugin.create_port(context, {'port': port_data})
 
 
-class _DelManager(object):
-    def __init__(self):
-        self.delete_on_error = True
-
-
 @contextlib.contextmanager
 def delete_port_on_error(core_plugin, context, port_id):
-    mgr = _DelManager()
     try:
-        yield mgr
+        yield
     except Exception:
         with excutils.save_and_reraise_exception():
             try:
-                if mgr.delete_on_error:
-                    core_plugin.delete_port(context, port_id,
-                                            l3_port_check=False)
+                core_plugin.delete_port(context, port_id,
+                                        l3_port_check=False)
             except Exception:
-                LOG.exception(_LE("Failed to cleanup port: %s"), port_id)
+                LOG.exception(_LE("Failed to delete port: %s"), port_id)
+
+
+@contextlib.contextmanager
+def update_port_on_error(core_plugin, context, port_id, revert_value):
+    try:
+        yield
+    except Exception:
+        with excutils.save_and_reraise_exception():
+            try:
+                core_plugin.update_port(context, port_id,
+                                        {'port': revert_value})
+            except Exception:
+                LOG.exception(_LE("Failed to update port: %s"), port_id)
 
 
 def get_interface_name(name, prefix='', max_len=n_const.DEVICE_NAME_MAX_LEN):

neutron/tests/unit/plugins/common/test_utils.py

@@ -18,6 +18,7 @@ import mock
 from neutron_lib import constants
 import testtools
 
+from neutron.db import l3_db
 from neutron.plugins.common import utils
 from neutron.tests import base
 
@@ -81,14 +82,6 @@ class TestUtils(base.BaseTestCase):
         core_plugin.delete_port.assert_called_once_with(context, port_id,
                                                         l3_port_check=False)
 
-    def test_delete_port_on_error_no_delete(self):
-        core_plugin, context = mock.Mock(), mock.Mock()
-        with testtools.ExpectedException(ValueError):
-            with utils.delete_port_on_error(core_plugin, context, 1) as cm:
-                cm.delete_on_error = False
-                raise ValueError()
-        self.assertFalse(core_plugin.delete_port.called)
-
     def test_delete_port_on_error_fail_port_delete(self):
         core_plugin, context = mock.Mock(), mock.Mock()
         core_plugin.delete_port.side_effect = TypeError()
@@ -98,3 +91,15 @@ class TestUtils(base.BaseTestCase):
                 raise ValueError()
         core_plugin.delete_port.assert_called_once_with(context, port_id,
                                                         l3_port_check=False)
+
+    @mock.patch.object(l3_db.L3_NAT_dbonly_mixin, '_check_router_port')
+    def test_update_port_on_error(self, mock_check):
+        core_plugin, context = mock.Mock(), mock.Mock()
+        port = mock_check.return_value = {'device_owner': 'xxxxxxxx'}
+        revert_value = {'device_id': '', 'device_owner': port['device_owner']}
+        with testtools.ExpectedException(ValueError):
+            with utils.update_port_on_error(core_plugin,
+                                            context, 1, revert_value):
+                raise ValueError()
+        core_plugin.update_port.assert_called_once_with(
+            context, 1, {'port': revert_value})