Merge "Rollback router intf port update if csnat update fails"

neutron/db/l3_dvr_db.py

@@ -22,7 +22,7 @@ from oslo_log import log as logging
 from oslo_utils import excutils
 import six
 
-from neutron._i18n import _, _LI, _LW
+from neutron._i18n import _, _LE, _LI, _LW
 from neutron.callbacks import events
 from neutron.callbacks import exceptions
 from neutron.callbacks import registry
@@ -30,6 +30,7 @@ from neutron.callbacks import resources
 from neutron.common import constants as l3_const
 from neutron.common import utils as n_utils
 from neutron.db.allowed_address_pairs import models as addr_pair_db
+from neutron.db import api as db_api
 from neutron.db import l3_agentschedulers_db as l3_sched_db
 from neutron.db import l3_attrs_db
 from neutron.db import l3_db
@@ -352,9 +353,41 @@ class L3_NAT_with_dvr_db_mixin(l3_db.L3_NAT_db_mixin,
                 if cs_port:
                     fixed_ips = list(cs_port['port']['fixed_ips'])
                     fixed_ips.append(fixed_ip)
-                    updated_port = self._core_plugin.update_port(
-                        context.elevated(),
-                        cs_port['port_id'], {'port': {'fixed_ips': fixed_ips}})
+                    try:
+                        updated_port = self._core_plugin.update_port(
+                            context.elevated(),
+                            cs_port['port_id'],
+                            {'port': {'fixed_ips': fixed_ips}})
+                    except Exception:
+                        with excutils.save_and_reraise_exception():
+                            # we need to try to undo the updated router
+                            # interface from above so it's not out of sync
+                            # with the csnat port.
+                            # TODO(kevinbenton): switch to taskflow to manage
+                            # these rollbacks.
+                            @db_api.retry_db_errors
+                            def revert():
+                                # TODO(kevinbenton): even though we get the
+                                # port each time, there is a potential race
+                                # where we update the port with stale IPs if
+                                # another interface operation is occuring at
+                                # the same time. This can be fixed in the
+                                # future with a compare-and-swap style update
+                                # using the revision number of the port.
+                                p = self._core_plugin.get_port(
+                                    context.elevated(), port['id'])
+                                upd = {'port': {'fixed_ips': [
+                                    ip for ip in p['fixed_ips']
+                                    if ip['subnet_id'] != fixed_ip['subnet_id']
+                                ]}}
+                                self._core_plugin.update_port(
+                                    context.elevated(), port['id'], upd)
+                            try:
+                                revert()
+                            except Exception:
+                                LOG.exception(_LE("Failed to revert change "
+                                                  "to router port %s."),
+                                              port['id'])
                     LOG.debug("CSNAT port updated for IPv6 subnet: "
                               "%s", updated_port)
         router_interface_info = self._make_router_interface_info(

neutron/tests/unit/db/test_l3_dvr_db.py

@@ -17,6 +17,7 @@ import mock
 from neutron_lib import constants as l3_const
 from neutron_lib import exceptions
 from oslo_utils import uuidutils
+import testtools
 
 from neutron.common import constants as n_const
 from neutron import context
@@ -555,6 +556,45 @@ class L3DvrTestCase(test_db_base_plugin_v2.NeutronDbPluginV2TestCase):
                 self.mixin.manager = manager
                 return router, subnet_v4, subnet_v6
 
+    def test_undo_router_interface_change_on_csnat_error(self):
+        self._test_undo_router_interface_change_on_csnat_error(False)
+
+    def test_undo_router_interface_change_on_csnat_error_revert_failure(self):
+        self._test_undo_router_interface_change_on_csnat_error(True)
+
+    def _test_undo_router_interface_change_on_csnat_error(self, fail_revert):
+        router, subnet_v4, subnet_v6 = self._setup_router_with_v4_and_v6()
+        net = {'network': {'id': subnet_v6['subnet']['network_id'],
+                           'tenant_id': subnet_v6['subnet']['tenant_id']}}
+        orig_update = self.mixin._core_plugin.update_port
+
+        def update_port(*args, **kwargs):
+            # 1st port update is the interface, 2nd is csnat, 3rd is revert
+            # we want to simulate errors after the 1st
+            update_port.calls += 1
+            if update_port.calls == 2:
+                raise RuntimeError('csnat update failure')
+            if update_port.calls == 3 and fail_revert:
+                # this is to ensure that if the revert fails, the original
+                # exception is raised (not this ValueError)
+                raise ValueError('failure from revert')
+            return orig_update(*args, **kwargs)
+        update_port.calls = 0
+        self.mixin._core_plugin.update_port = update_port
+
+        with self.subnet(network=net, cidr='fe81::/64',
+                         gateway_ip='fe81::1', ip_version=6) as subnet2_v6:
+            with testtools.ExpectedException(RuntimeError):
+                self.mixin.add_router_interface(self.ctx, router['id'],
+                    {'subnet_id': subnet2_v6['subnet']['id']})
+            if fail_revert:
+                # a revert failure will mean the interface is still added
+                # so we can't re-add it
+                return
+            # starting over should work if first interface was cleaned up
+            self.mixin.add_router_interface(self.ctx, router['id'],
+                {'subnet_id': subnet2_v6['subnet']['id']})
+
     def test_remove_router_interface_csnat_ports_removal_with_ipv6(self):
         router, subnet_v4, subnet_v6 = self._setup_router_with_v4_and_v6()
         csnat_filters = {'device_owner':