diff --git a/neutron/db/l3_db.py b/neutron/db/l3_db.py index bd7421b98ea..b488eea83c3 100644 --- a/neutron/db/l3_db.py +++ b/neutron/db/l3_db.py @@ -368,17 +368,18 @@ class L3_NAT_dbonly_mixin(l3.RouterPluginBase, if not gw_port['fixed_ips']: LOG.debug('No IPs available for external network %s', network_id) - - with context.session.begin(subtransactions=True): - router.gw_port = self._core_plugin._get_port(context.elevated(), - gw_port['id']) - router_port = RouterPort( - router_id=router.id, - port_id=gw_port['id'], - port_type=DEVICE_OWNER_ROUTER_GW - ) - context.session.add(router) - context.session.add(router_port) + with p_utils.delete_port_on_error(self._core_plugin, + context.elevated(), gw_port['id']): + with context.session.begin(subtransactions=True): + router.gw_port = self._core_plugin._get_port( + context.elevated(), gw_port['id']) + router_port = RouterPort( + router_id=router.id, + port_id=gw_port['id'], + port_type=DEVICE_OWNER_ROUTER_GW + ) + context.session.add(router) + context.session.add(router_port) def _validate_gw_info(self, context, gw_port, info, ext_ips): network_id = info['network_id'] if info else None @@ -746,6 +747,7 @@ class L3_NAT_dbonly_mixin(l3.RouterPluginBase, # This should be True unless adding an IPv6 prefix to an existing port new_port = True + cleanup_port = False if add_by_port: port, subnets = self._add_interface_by_port( @@ -755,15 +757,19 @@ class L3_NAT_dbonly_mixin(l3.RouterPluginBase, else: port, subnets, new_port = self._add_interface_by_subnet( context, router, interface_info['subnet_id'], device_owner) + cleanup_port = new_port # only cleanup port we created if new_port: - with context.session.begin(subtransactions=True): - router_port = RouterPort( - port_id=port['id'], - router_id=router.id, - port_type=device_owner - ) - context.session.add(router_port) + with p_utils.delete_port_on_error(self._core_plugin, + context, port['id']) as delmgr: + delmgr.delete_on_error = cleanup_port + with context.session.begin(subtransactions=True): + router_port = RouterPort( + port_id=port['id'], + router_id=router.id, + port_type=device_owner + ) + context.session.add(router_port) gw_ips = [] gw_network_id = None diff --git a/neutron/db/l3_dvr_db.py b/neutron/db/l3_dvr_db.py index e55be908eb7..2cc082fd716 100644 --- a/neutron/db/l3_dvr_db.py +++ b/neutron/db/l3_dvr_db.py @@ -307,6 +307,7 @@ class L3_NAT_with_dvr_db_mixin(l3_db.L3_NAT_db_mixin, # This should be True unless adding an IPv6 prefix to an existing port new_port = True + cleanup_port = False if add_by_port: port, subnets = self._add_interface_by_port( @@ -314,31 +315,27 @@ class L3_NAT_with_dvr_db_mixin(l3_db.L3_NAT_db_mixin, elif add_by_sub: port, subnets, new_port = self._add_interface_by_subnet( context, router, interface_info['subnet_id'], device_owner) + cleanup_port = new_port subnet = subnets[0] if new_port: - if router.extra_attributes.distributed and router.gw_port: - try: + with p_utils.delete_port_on_error(self._core_plugin, + context, port['id']) as delmgr: + if router.extra_attributes.distributed and router.gw_port: admin_context = context.elevated() self._add_csnat_router_interface_port( admin_context, router, port['network_id'], port['fixed_ips'][-1]['subnet_id']) - except Exception: - with excutils.save_and_reraise_exception(): - # we need to preserve the original state prior - # the request by rolling back the port creation - # that led to new_port=True - self._core_plugin.delete_port( - admin_context, port['id'], l3_port_check=False) - with context.session.begin(subtransactions=True): - router_port = l3_db.RouterPort( - port_id=port['id'], - router_id=router.id, - port_type=device_owner - ) - context.session.add(router_port) + delmgr.delete_on_error = cleanup_port + with context.session.begin(subtransactions=True): + router_port = l3_db.RouterPort( + port_id=port['id'], + router_id=router.id, + port_type=device_owner + ) + context.session.add(router_port) # NOTE: For IPv6 additional subnets added to the same # network we need to update the CSNAT port with respective diff --git a/neutron/plugins/common/utils.py b/neutron/plugins/common/utils.py index 8255b3e3632..3e6aca81cb5 100644 --- a/neutron/plugins/common/utils.py +++ b/neutron/plugins/common/utils.py @@ -16,6 +16,7 @@ Common utilities and helper functions for OpenStack Networking Plugins. """ +import contextlib import hashlib from neutron_lib import constants as n_const @@ -23,9 +24,10 @@ from neutron_lib import exceptions from oslo_config import cfg from oslo_log import log as logging from oslo_utils import encodeutils +from oslo_utils import excutils import webob.exc -from neutron._i18n import _, _LI +from neutron._i18n import _, _LE, _LI from neutron.api.v2 import attributes from neutron.callbacks import events from neutron.callbacks import registry @@ -195,6 +197,26 @@ 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 + 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) + except Exception: + LOG.exception(_LE("Failed to cleanup port: %s"), port_id) + + def get_interface_name(name, prefix='', max_len=n_const.DEVICE_NAME_MAX_LEN): """Construct an interface name based on the prefix and name. diff --git a/neutron/tests/unit/extensions/test_l3_ext_gw_mode.py b/neutron/tests/unit/extensions/test_l3_ext_gw_mode.py index a6423c4da90..4298c9c7a2b 100644 --- a/neutron/tests/unit/extensions/test_l3_ext_gw_mode.py +++ b/neutron/tests/unit/extensions/test_l3_ext_gw_mode.py @@ -23,6 +23,7 @@ from oslo_utils import uuidutils import testscenarios from webob import exc +from neutron import context as nctx from neutron.db import api as db_api from neutron.db import external_net_db from neutron.db import l3_db @@ -30,6 +31,7 @@ from neutron.db import l3_gwmode_db from neutron.db import models_v2 from neutron.extensions import l3 from neutron.extensions import l3_ext_gw_mode +from neutron import manager from neutron.tests import base from neutron.tests.unit.db import test_db_base_plugin_v2 from neutron.tests.unit.extensions import test_l3 @@ -390,6 +392,20 @@ class ExtGwModeIntTestCase(test_db_base_plugin_v2.NeutronDbPluginV2TestCase, expected_code=expected_code, neutron_context=neutron_context) + def test_router_gateway_set_fail_after_port_create(self): + with self.router() as r, self.subnet() as s: + ext_net_id = s['subnet']['network_id'] + self._set_net_external(ext_net_id) + plugin = manager.NeutronManager.get_plugin() + with mock.patch.object(plugin, '_get_port', + side_effect=ValueError()): + self._set_router_external_gateway(r['router']['id'], + ext_net_id, + expected_code=500) + ports = [p for p in plugin.get_ports(nctx.get_admin_context()) + if p['device_owner'] == l3_db.DEVICE_OWNER_ROUTER_GW] + self.assertFalse(ports) + def test_router_gateway_set_retry(self): with self.router() as r, self.subnet() as s: ext_net_id = s['subnet']['network_id'] diff --git a/neutron/tests/unit/plugins/common/test_utils.py b/neutron/tests/unit/plugins/common/test_utils.py index f61444192c9..16e0bde0dc0 100644 --- a/neutron/tests/unit/plugins/common/test_utils.py +++ b/neutron/tests/unit/plugins/common/test_utils.py @@ -16,6 +16,7 @@ import hashlib import mock from neutron_lib import constants +import testtools from neutron.plugins.common import utils from neutron.tests import base @@ -70,3 +71,30 @@ class TestUtils(base.BaseTestCase): self.assertEqual(12, len(utils.get_interface_name(LONG_NAME1, prefix="pre-", max_len=12))) + + def test_delete_port_on_error(self): + core_plugin, context = mock.Mock(), mock.Mock() + port_id = 'pid' + with testtools.ExpectedException(ValueError): + with utils.delete_port_on_error(core_plugin, context, port_id): + raise ValueError() + 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() + port_id = 'pid' + with testtools.ExpectedException(ValueError): + with utils.delete_port_on_error(core_plugin, context, port_id): + raise ValueError() + core_plugin.delete_port.assert_called_once_with(context, port_id, + l3_port_check=False)