diff --git a/neutron/db/l3_db.py b/neutron/db/l3_db.py index ec23c4b1fa4..2c1133e78c9 100644 --- a/neutron/db/l3_db.py +++ b/neutron/db/l3_db.py @@ -925,21 +925,13 @@ class L3_NAT_dbonly_mixin(l3.RouterPluginBase, def _remove_interface_by_port(self, context, router_id, port_id, subnet_id, owner): - obj = l3_obj.RouterPort.get_object( - context, - port_id=port_id, - router_id=router_id, - port_type=owner - ) - if obj: - try: - port = self._core_plugin.get_port(context, obj.port_id) - except n_exc.PortNotFound: - raise l3_exc.RouterInterfaceNotFound( - router_id=router_id, port_id=port_id) - else: + ports = port_obj.Port.get_ports_by_router_and_port( + context, router_id, owner, port_id) + if len(ports) < 1: raise l3_exc.RouterInterfaceNotFound( router_id=router_id, port_id=port_id) + + port = ports[0] port_subnet_ids = [fixed_ip['subnet_id'] for fixed_ip in port['fixed_ips']] if subnet_id and subnet_id not in port_subnet_ids: @@ -952,46 +944,41 @@ class L3_NAT_dbonly_mixin(l3.RouterPluginBase, context, router_id, port_subnet_id) self._core_plugin.delete_port(context, port['id'], l3_port_check=False) - return (port, subnets) + return port, subnets def _remove_interface_by_subnet(self, context, router_id, subnet_id, owner): self._confirm_router_interface_not_in_use( context, router_id, subnet_id) subnet = self._core_plugin.get_subnet(context, subnet_id) + ports = port_obj.Port.get_ports_by_router_and_network( + context, router_id, owner, subnet['network_id']) - try: - ports = port_obj.Port.get_ports_by_router( - context, router_id, owner, subnet) - - for p in ports: - try: - p = self._core_plugin.get_port(context, p.id) - except n_exc.PortNotFound: - continue - port_subnets = [fip['subnet_id'] for fip in p['fixed_ips']] - if subnet_id in port_subnets and len(port_subnets) > 1: - # multiple prefix port - delete prefix from port - fixed_ips = [dict(fip) for fip in p['fixed_ips'] - if fip['subnet_id'] != subnet_id] - self._core_plugin.update_port( - context, p['id'], {'port': {'fixed_ips': fixed_ips}}) - return (p, [subnet]) - elif subnet_id in port_subnets: - # only one subnet on port - delete the port - self._core_plugin.delete_port(context, p['id'], - l3_port_check=False) - return (p, [subnet]) - except exc.NoResultFound: - pass + for p in ports: + try: + p = self._core_plugin.get_port(context, p.id) + except n_exc.PortNotFound: + continue + port_subnets = [fip['subnet_id'] for fip in p['fixed_ips']] + if subnet_id in port_subnets and len(port_subnets) > 1: + # multiple prefix port - delete prefix from port + fixed_ips = [dict(fip) for fip in p['fixed_ips'] + if fip['subnet_id'] != subnet_id] + self._core_plugin.update_port( + context, p['id'], {'port': {'fixed_ips': fixed_ips}}) + return (p, [subnet]) + elif subnet_id in port_subnets: + # only one subnet on port - delete the port + self._core_plugin.delete_port(context, p['id'], + l3_port_check=False) + return (p, [subnet]) raise l3_exc.RouterInterfaceNotFoundForSubnet( router_id=router_id, subnet_id=subnet_id) @db_api.retry_if_session_inactive() def remove_router_interface(self, context, router_id, interface_info): - remove_by_port, remove_by_subnet = ( - self._validate_interface_info(interface_info, for_removal=True) - ) + remove_by_port, _ = self._validate_interface_info(interface_info, + for_removal=True) port_id = interface_info.get('port_id') subnet_id = interface_info.get('subnet_id') device_owner = self._get_device_owner(context, router_id) @@ -999,9 +986,6 @@ class L3_NAT_dbonly_mixin(l3.RouterPluginBase, port, subnets = self._remove_interface_by_port(context, router_id, port_id, subnet_id, device_owner) - # remove_by_subnet is not used here, because the validation logic of - # _validate_interface_info ensures that at least one of remote_by_* - # is True. else: port, subnets = self._remove_interface_by_subnet( context, router_id, subnet_id, device_owner) diff --git a/neutron/objects/ports.py b/neutron/objects/ports.py index e8f9de4140c..dbe0fe4c94c 100644 --- a/neutron/objects/ports.py +++ b/neutron/objects/ports.py @@ -15,6 +15,7 @@ import netaddr from neutron_lib import constants from neutron_lib.utils import net as net_utils +from oslo_log import log as logging from oslo_utils import versionutils from oslo_versionedobjects import fields as obj_fields @@ -28,6 +29,8 @@ from neutron.objects.db import api as obj_db_api from neutron.objects.qos import binding from neutron.plugins.ml2 import models as ml2_models +LOG = logging.getLogger(__name__) + class PortBindingBase(base.NeutronDbObject): @@ -508,15 +511,65 @@ class Port(base.NeutronDbObject): primitive.pop('qos_network_policy_id', None) @classmethod - def get_ports_by_router(cls, context, router_id, owner, subnet): - rport_qry = context.session.query(models_v2.Port).join( - l3.RouterPort) - ports = rport_qry.filter( - l3.RouterPort.router_id == router_id, - l3.RouterPort.port_type == owner, - models_v2.Port.network_id == subnet['network_id'] - ) - return [cls._load_object(context, db_obj) for db_obj in ports.all()] + def get_ports_by_router_and_network(cls, context, router_id, owner, + network_id): + """Returns port objects filtering by router ID, owner and network ID""" + rports_filter = (models_v2.Port.network_id == network_id, ) + router_filter = (models_v2.Port.network_id == network_id, ) + return cls._get_ports_by_router(context, router_id, owner, + rports_filter, router_filter) + + @classmethod + def get_ports_by_router_and_port(cls, context, router_id, owner, port_id): + """Returns port objects filtering by router ID, owner and port ID""" + rports_filter = (l3.RouterPort.port_id == port_id, ) + router_filter = (models_v2.Port.id == port_id, ) + return cls._get_ports_by_router(context, router_id, owner, + rports_filter, router_filter) + + @classmethod + def _get_ports_by_router(cls, context, router_id, owner, rports_filter, + router_filter): + """Returns port objects filtering by router id and owner + + The method will receive extra filters depending of the caller (filter + by network or filter by port). + + The ports are retrieved using: + - The RouterPort registers. Each time a port is assigned to a router, + a new RouterPort register is added to the DB. + - The port owner and device_id information. + + Both searches should return the same result. If not, a warning message + is logged and the port list to be returned is completed with the + missing ones. + """ + rports_filter += (l3.RouterPort.router_id == router_id, + l3.RouterPort.port_type == owner) + router_filter += (models_v2.Port.device_id == router_id, + models_v2.Port.device_owner == owner) + + ports = context.session.query(models_v2.Port).join( + l3.RouterPort).filter(*rports_filter) + ports_rports = [cls._load_object(context, db_obj) + for db_obj in ports.all()] + + ports = context.session.query(models_v2.Port).filter(*router_filter) + ports_router = [cls._load_object(context, db_obj) + for db_obj in ports.all()] + + ports_rports_ids = {p.id for p in ports_rports} + ports_router_ids = {p.id for p in ports_router} + missing_port_ids = ports_router_ids - ports_rports_ids + if missing_port_ids: + LOG.warning('The following ports, assigned to router ' + '%(router_id)s, do not have a "routerport" register: ' + '%(port_ids)s', {'router_id': router_id, + 'port_ids': missing_port_ids}) + port_objs = [p for p in ports_router if p.id in missing_port_ids] + ports_rports += port_objs + + return ports_rports @classmethod def get_ports_ids_by_security_groups(cls, context, security_group_ids, diff --git a/neutron/tests/unit/db/test_l3_db.py b/neutron/tests/unit/db/test_l3_db.py index 7c47d4671fc..42bc065893f 100644 --- a/neutron/tests/unit/db/test_l3_db.py +++ b/neutron/tests/unit/db/test_l3_db.py @@ -22,6 +22,7 @@ from neutron_lib import constants as n_const from neutron_lib import context from neutron_lib import exceptions as n_exc from neutron_lib.exceptions import l3 as l3_exc +from neutron_lib.plugins import constants as plugin_constants from neutron_lib.plugins import directory from neutron_lib.plugins import utils as plugin_utils from oslo_utils import uuidutils @@ -30,8 +31,12 @@ import testtools from neutron.db import l3_db from neutron.db.models import l3 as l3_models from neutron.objects import base as base_obj +from neutron.objects import network as network_obj +from neutron.objects import ports as port_obj from neutron.objects import router as l3_obj +from neutron.objects import subnet as subnet_obj from neutron.tests import base +from neutron.tests.unit.db import test_db_base_plugin_v2 class TestL3_NAT_dbonly_mixin(base.BaseTestCase): @@ -377,3 +382,148 @@ class L3_NAT_db_mixin(base.BaseTestCase): self.assertRaises( n_exc.BadRequest, self.db.add_router_interface, mock.Mock(), router_db.id) + + +class FakeL3Plugin(l3_db.L3_NAT_dbonly_mixin): + pass + + +class L3TestCase(test_db_base_plugin_v2.NeutronDbPluginV2TestCase): + + GET_PORTS_BY_ROUTER_MSG = ( + 'The following ports, assigned to router %(router_id)s, do not have a ' + '"routerport" register: %(port_ids)s') + + def setUp(self, *args, **kwargs): + super(L3TestCase, self).setUp(plugin='ml2') + self.core_plugin = directory.get_plugin() + self.ctx = context.get_admin_context() + self.mixin = FakeL3Plugin() + directory.add_plugin(plugin_constants.L3, self.mixin) + self.network = self.create_network() + self.subnets = [] + self.subnets.append(self.create_subnet(self.network, '1.1.1.1', + '1.1.1.0/24')) + self.subnets.append(self.create_subnet(self.network, '1.1.2.1', + '1.1.2.0/24')) + router = {'router': {'name': 'foo_router', 'admin_state_up': True, + 'tenant_id': 'foo_tenant'}} + self.router = self.create_router(router) + self.ports = [] + for subnet in self.subnets: + ipa = str(netaddr.IPNetwork(subnet['subnet']['cidr']).ip + 10) + fixed_ips = [{'subnet_id': subnet['subnet']['id'], + 'ip_address': ipa}] + self.ports.append(self.create_port( + self.network['network']['id'], {'fixed_ips': fixed_ips})) + self.addCleanup(self._clean_objs) + + def _clean_objs(self): + port_obj.Port.delete_objects( + self.ctx, network_id=self.network['network']['id']) + subnet_obj.Subnet.delete_objects( + self.ctx, network_id=self.network['network']['id']) + network_obj.Network.get_object( + self.ctx, id=self.network['network']['id']).delete() + l3_obj.Router.get_object(self.ctx, id=self.router['id']).delete() + + def create_router(self, router): + with self.ctx.session.begin(subtransactions=True): + return self.mixin.create_router(self.ctx, router) + + def create_port(self, net_id, port_info): + with self.ctx.session.begin(subtransactions=True): + return self._make_port(self.fmt, net_id, **port_info) + + def create_network(self, name=None, **kwargs): + name = name or 'network1' + with self.ctx.session.begin(subtransactions=True): + return self._make_network(self.fmt, name, True, **kwargs) + + def create_subnet(self, network, gateway, cidr, **kwargs): + with self.ctx.session.begin(subtransactions=True): + return self._make_subnet(self.fmt, network, gateway, cidr, + **kwargs) + + def _add_router_interfaces(self): + return [self.mixin.add_router_interface( + self.ctx, self.router['id'], + interface_info={'port_id': port['port']['id']}) + for port in self.ports] + + def _check_routerports(self, ri_statuses): + port_ids = [] + for idx, ri_status in enumerate(ri_statuses): + rp_obj = l3_obj.RouterPort.get_object( + self.ctx, port_id=self.ports[idx]['port']['id'], + router_id=self.router['id']) + if ri_status: + self.assertEqual(self.ports[idx]['port']['id'], rp_obj.port_id) + port_ids.append(rp_obj.port_id) + else: + self.assertIsNone(rp_obj) + + _router_obj = l3_obj.Router.get_object(self.ctx, id=self.router['id']) + router_port_ids = [rp.port_id for rp in + _router_obj.db_obj.attached_ports] + self.assertEqual(sorted(port_ids), sorted(router_port_ids)) + + @mock.patch.object(port_obj, 'LOG') + def test_remove_router_interface_by_port(self, mock_log): + self._add_router_interfaces() + self._check_routerports((True, True)) + + interface_info = {'port_id': self.ports[0]['port']['id']} + self.mixin.remove_router_interface(self.ctx, self.router['id'], + interface_info) + mock_log.warning.assert_not_called() + self._check_routerports((False, True)) + + @mock.patch.object(port_obj, 'LOG') + def test_remove_router_interface_by_port_removed_rport(self, mock_log): + self._add_router_interfaces() + self._check_routerports((True, True)) + + rp_obj = l3_obj.RouterPort.get_object( + self.ctx, router_id=self.router['id'], + port_id=self.ports[0]['port']['id']) + rp_obj.delete() + + interface_info = {'port_id': self.ports[0]['port']['id']} + self.mixin.remove_router_interface(self.ctx, self.router['id'], + interface_info) + msg_vars = {'router_id': self.router['id'], + 'port_ids': {self.ports[0]['port']['id']}} + mock_log.warning.assert_called_once_with(self.GET_PORTS_BY_ROUTER_MSG, + msg_vars) + self._check_routerports((False, True)) + + @mock.patch.object(port_obj, 'LOG') + def test_remove_router_interface_by_subnet(self, mock_log): + self._add_router_interfaces() + self._check_routerports((True, True)) + + interface_info = {'subnet_id': self.subnets[1]['subnet']['id']} + self.mixin.remove_router_interface(self.ctx, self.router['id'], + interface_info) + mock_log.warning.not_called_once() + self._check_routerports((True, False)) + + @mock.patch.object(port_obj, 'LOG') + def test_remove_router_interface_by_subnet_removed_rport(self, mock_log): + self._add_router_interfaces() + self._check_routerports((True, True)) + + rp_obj = l3_obj.RouterPort.get_object( + self.ctx, router_id=self.router['id'], + port_id=self.ports[0]['port']['id']) + rp_obj.delete() + + interface_info = {'subnet_id': self.subnets[0]['subnet']['id']} + self.mixin.remove_router_interface(self.ctx, self.router['id'], + interface_info) + msg_vars = {'router_id': self.router['id'], + 'port_ids': {self.ports[0]['port']['id']}} + mock_log.warning.assert_called_once_with(self.GET_PORTS_BY_ROUTER_MSG, + msg_vars) + self._check_routerports((False, True))