From: Ivar Lazzaro Date: Fri, 22 Aug 2014 01:35:02 +0000 (-0700) Subject: Apic drivers enhancements (second approach): L3 refactor X-Git-Url: https://review.fuel-infra.org/gitweb?a=commitdiff_plain;h=34a05588856e21d85826be6ceca4b1a964810bb1;p=openstack-build%2Fneutron-build.git Apic drivers enhancements (second approach): L3 refactor - refactor to leverage Client's transactional capabilities - General refactor to improve the driver's reliability Implements blueprint: apic-driver-enhancements Change-Id: I5a19039e82988a0570622bc1ddb1429e9833d478 --- diff --git a/neutron/db/migration/alembic_migrations/versions/86d6d9776e2b_cisco_apic_driver_update_l3.py b/neutron/db/migration/alembic_migrations/versions/86d6d9776e2b_cisco_apic_driver_update_l3.py new file mode 100644 index 000000000..d753b33fa --- /dev/null +++ b/neutron/db/migration/alembic_migrations/versions/86d6d9776e2b_cisco_apic_driver_update_l3.py @@ -0,0 +1,64 @@ +# Copyright 2014 OpenStack Foundation +# +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. +# + +"""Cisco APIC Mechanism Driver + +Revision ID: 86d6d9776e2b +Revises: 236b90af57abg +Create Date: 2014-04-23 09:27:08.177021 + +""" + +# revision identifiers, used by Alembic. +revision = '86d6d9776e2b' +down_revision = '236b90af57ab' + + +from alembic import op +import sqlalchemy as sa + + +def upgrade(active_plugins=None, options=None): + + op.drop_table('cisco_ml2_apic_contracts') + op.drop_table('cisco_ml2_apic_epgs') + + op.create_table( + 'cisco_ml2_apic_contracts', + sa.Column('tenant_id', sa.String(length=255)), + sa.Column('router_id', sa.String(length=64), nullable=False), + sa.ForeignKeyConstraint(['router_id'], ['routers.id']), + sa.PrimaryKeyConstraint('router_id')) + + +def downgrade(active_plugins=None, options=None): + + op.drop_table('cisco_ml2_apic_contracts') + + op.create_table( + 'cisco_ml2_apic_epgs', + sa.Column('network_id', sa.String(length=255), nullable=False), + sa.Column('epg_id', sa.String(length=64), nullable=False), + sa.Column('segmentation_id', sa.String(length=64), nullable=False), + sa.Column('provider', sa.Boolean(), server_default=sa.sql.false(), + nullable=False), + sa.PrimaryKeyConstraint('network_id')) + + op.create_table( + 'cisco_ml2_apic_contracts', + sa.Column('tenant_id', sa.String(length=255)), + sa.Column('contract_id', sa.String(length=64), nullable=False), + sa.Column('filter_id', sa.String(length=64), nullable=False), + sa.PrimaryKeyConstraint('tenant_id')) diff --git a/neutron/db/migration/alembic_migrations/versions/HEAD b/neutron/db/migration/alembic_migrations/versions/HEAD index dbff3d53e..afbbf75d3 100644 --- a/neutron/db/migration/alembic_migrations/versions/HEAD +++ b/neutron/db/migration/alembic_migrations/versions/HEAD @@ -1 +1 @@ -236b90af57ab +86d6d9776e2b diff --git a/neutron/plugins/ml2/drivers/cisco/apic/apic_model.py b/neutron/plugins/ml2/drivers/cisco/apic/apic_model.py index 9111b1546..256369f25 100644 --- a/neutron/plugins/ml2/drivers/cisco/apic/apic_model.py +++ b/neutron/plugins/ml2/drivers/cisco/apic/apic_model.py @@ -17,7 +17,6 @@ import sqlalchemy as sa from sqlalchemy import orm -from sqlalchemy import sql from neutron.db import api as db_api from neutron.db import model_base @@ -26,29 +25,20 @@ from neutron.db import models_v2 from neutron.plugins.ml2 import models as models_ml2 -class NetworkEPG(model_base.BASEV2): +class RouterContract(model_base.BASEV2, models_v2.HasTenant): - """EPG's created on the apic per network.""" + """Contracts created on the APIC. - __tablename__ = 'cisco_ml2_apic_epgs' - - network_id = sa.Column(sa.String(255), nullable=False, primary_key=True) - epg_id = sa.Column(sa.String(64), nullable=False) - segmentation_id = sa.Column(sa.String(64), nullable=False) - provider = sa.Column(sa.Boolean, default=False, server_default=sql.false(), - nullable=False) - - -class TenantContract(model_base.BASEV2): - - """Contracts (and Filters) created on the APIC.""" + tenant_id represents the owner (APIC side) of the contract. + router_id is the UUID of the router (Neutron side) this contract is + referring to. + """ __tablename__ = 'cisco_ml2_apic_contracts' - # Cannot use HasTenant since we need to set nullable=False - tenant_id = sa.Column(sa.String(255), nullable=False, primary_key=True) - contract_id = sa.Column(sa.String(64), nullable=False) - filter_id = sa.Column(sa.String(64), nullable=False) + router_id = sa.Column(sa.String(64), sa.ForeignKey('routers.id', + ondelete='CASCADE'), + primary_key=True) class HostLink(model_base.BASEV2): @@ -82,72 +72,37 @@ class ApicDbModel(object): def __init__(self): self.session = db_api.get_session() - def get_provider_contract(self): - """Returns provider EPG from the DB if found.""" - return self.session.query(NetworkEPG).filter_by( - provider=True).first() - - def set_provider_contract(self, epg_id): - """Sets an EPG to be a contract provider.""" - epg = self.session.query(NetworkEPG).filter_by( - epg_id=epg_id).first() - if epg: - epg.provider = True - self.session.merge(epg) - self.session.flush() - - def unset_provider_contract(self, epg_id): - """Sets an EPG to be a contract consumer.""" - epg = self.session.query(NetworkEPG).filter_by( - epg_id=epg_id).first() - if epg: - epg.provider = False - self.session.merge(epg) - self.session.flush() - - def get_an_epg(self, exception): - """Returns an EPG from the DB that does not match the id specified.""" - return self.session.query(NetworkEPG).filter( - NetworkEPG.epg_id != exception).first() - - def get_epg_for_network(self, network_id): - """Returns an EPG for a give neutron network.""" - return self.session.query(NetworkEPG).filter_by( - network_id=network_id).first() - - def write_epg_for_network(self, network_id, epg_uid, segmentation_id='1'): - """Stores EPG details for a network. - - NOTE: Segmentation_id is just a placeholder currently, it will be - populated with a proper segment id once segmentation mgmt is - moved to the APIC. - """ - epg = NetworkEPG(network_id=network_id, epg_id=epg_uid, - segmentation_id=segmentation_id) - self.session.add(epg) - self.session.flush() - return epg - - def delete_epg(self, epg): - """Deletes an EPG from the DB.""" - self.session.delete(epg) - self.session.flush() - - def get_contract_for_tenant(self, tenant_id): - """Returns the specified tenant's contract.""" - return self.session.query(TenantContract).filter_by( - tenant_id=tenant_id).first() - - def write_contract_for_tenant(self, tenant_id, contract_id, filter_id): - """Stores a new contract for the given tenant.""" - contract = TenantContract(tenant_id=tenant_id, - contract_id=contract_id, - filter_id=filter_id) - self.session.add(contract) - self.session.flush() + def get_contract_for_router(self, router_id): + """Returns the specified router's contract.""" + return self.session.query(RouterContract).filter_by( + router_id=router_id).first() + def write_contract_for_router(self, tenant_id, router_id): + """Stores a new contract for the given tenant.""" + contract = RouterContract(tenant_id=tenant_id, + router_id=router_id) + with self.session.begin(subtransactions=True): + self.session.add(contract) return contract + def update_contract_for_router(self, tenant_id, router_id): + with self.session.begin(subtransactions=True): + contract = self.session.query(RouterContract).filter_by( + router_id=router_id).with_lockmode('update').first() + if contract: + contract.tenant_id = tenant_id + self.session.merge(contract) + else: + self.write_contract_for_router(tenant_id, router_id) + + def delete_contract_for_router(self, router_id): + with self.session.begin(subtransactions=True): + try: + self.session.query(RouterContract).filter_by( + router_id=router_id).delete() + except orm.exc.NoResultFound: + return + def add_hostlink(self, host, ifname, ifmac, swid, module, port): link = HostLink(host=host, ifname=ifname, ifmac=ifmac, swid=swid, module=module, port=port) @@ -216,7 +171,8 @@ class ApicDbModel(object): def update_apic_name(self, neutron_id, neutron_type, apic_name): with self.session.begin(subtransactions=True): name = self.session.query(ApicName).filter_by( - neutron_id=neutron_id, neutron_type=neutron_type).first() + neutron_id=neutron_id, + neutron_type=neutron_type).with_lockmode('update').first() if name: name.apic_name = apic_name self.session.merge(name) diff --git a/neutron/services/l3_router/l3_apic.py b/neutron/services/l3_router/l3_apic.py index 75622fbc7..f55001bb9 100644 --- a/neutron/services/l3_router/l3_apic.py +++ b/neutron/services/l3_router/l3_apic.py @@ -14,32 +14,40 @@ # under the License. # # @author: Arvind Somya (asomya@cisco.com), Cisco Systems Inc. +# @author: Ivar Lazzaro (ivarlazzaro@gmail.com), Cisco Systems Inc. + +from apicapi import apic_mapper from neutron.db import db_base_plugin_v2 from neutron.db import extraroute_db -from neutron.db import l3_gwmode_db +from neutron.db import l3_dvr_db from neutron.openstack.common import excutils -from neutron.openstack.common import log as logging from neutron.plugins.common import constants -from neutron.plugins.ml2.drivers.cisco.apic import mechanism_apic as ma -LOG = logging.getLogger(__name__) +from neutron.plugins.ml2.drivers.cisco.apic import mechanism_apic class ApicL3ServicePlugin(db_base_plugin_v2.NeutronDbPluginV2, - extraroute_db.ExtraRoute_db_mixin, - l3_gwmode_db.L3_NAT_db_mixin): - """Implementation of the APIC L3 Router Service Plugin. - - This class implements a L3 service plugin that provides - internal gateway functionality for the Cisco APIC (Application - Policy Infrastructure Controller). - """ + l3_dvr_db.L3_NAT_with_dvr_db_mixin, + extraroute_db.ExtraRoute_db_mixin): supported_extension_aliases = ["router", "ext-gw-mode", "extraroute"] def __init__(self): super(ApicL3ServicePlugin, self).__init__() - self.manager = ma.APICMechanismDriver.get_apic_manager() + self.manager = mechanism_apic.APICMechanismDriver.get_apic_manager() + self.name_mapper = self.manager.apic_mapper + self.manager.ensure_infra_created_on_apic() + self.manager.ensure_bgp_pod_policy_created_on_apic() + + def _map_names(self, context, + tenant_id, router_id, net_id, subnet_id): + context._plugin = self + with apic_mapper.mapper_context(context) as ctx: + atenant_id = tenant_id and self.name_mapper.tenant(ctx, tenant_id) + arouter_id = router_id and self.name_mapper.router(ctx, router_id) + anet_id = net_id and self.name_mapper.network(ctx, net_id) + asubnet_id = subnet_id and self.name_mapper.subnet(ctx, subnet_id) + return atenant_id, arouter_id, anet_id, asubnet_id @staticmethod def get_plugin_type(): @@ -50,79 +58,99 @@ class ApicL3ServicePlugin(db_base_plugin_v2.NeutronDbPluginV2, """Returns string description of the plugin.""" return _("L3 Router Service Plugin for basic L3 using the APIC") - def _add_epg_to_contract(self, tenant_id, epg, contract): - """Add an End Point Group(EPG) to a contract as provider/consumer.""" - if self.manager.db.get_provider_contract(): - # Set this network's EPG as a consumer - self.manager.set_contract_for_epg(tenant_id, epg.epg_id, - contract.contract_id) - else: - # Set this network's EPG as a provider - self.manager.set_contract_for_epg(tenant_id, epg.epg_id, - contract.contract_id, - provider=True) + def add_router_interface_postcommit(self, context, router_id, + interface_info): + # Update router's state first + router = self.get_router(context, router_id) + self.update_router_postcommit(context, router) - def add_router_interface(self, context, router_id, interface_info): - """Attach a subnet to a router.""" - tenant_id = context.tenant_id - subnet_id = interface_info['subnet_id'] - LOG.debug("Attaching subnet %(subnet_id)s to " - "router %(router_id)s" % {'subnet_id': subnet_id, - 'router_id': router_id}) - - # Get network for this subnet - subnet = self.get_subnet(context, subnet_id) - network_id = subnet['network_id'] - net_name = self.get_network(context, network_id)['name'] - - # Setup tenant filters and contracts - contract = self.manager.create_tenant_contract(tenant_id) - - # Check for a provider EPG - epg = self.manager.ensure_epg_created_for_network(tenant_id, - network_id, - net_name) - self._add_epg_to_contract(tenant_id, epg, contract) - - # Create DB port - try: - return super(ApicL3ServicePlugin, self).add_router_interface( - context, router_id, interface_info) - except Exception: - LOG.error(_("Error attaching subnet %(subnet_id)s to " - "router %(router_id)s") % {'subnet_id': subnet_id, - 'router_id': router_id}) - with excutils.save_and_reraise_exception(): - self.manager.delete_contract_for_epg(tenant_id, epg.epg_id, - contract.contract_id, - provider=epg.provider) - - def remove_router_interface(self, context, router_id, interface_info): - """Detach a subnet from a router.""" - tenant_id = context.tenant_id + # Add router interface if 'subnet_id' in interface_info: subnet = self.get_subnet(context, interface_info['subnet_id']) network_id = subnet['network_id'] + tenant_id = subnet['tenant_id'] else: port = self.get_port(context, interface_info['port_id']) network_id = port['network_id'] + tenant_id = port['tenant_id'] - # Get network for this subnet - network = self.get_network(context, network_id) + # Map openstack IDs to APIC IDs + atenant_id, arouter_id, anetwork_id, _ = self._map_names( + context, tenant_id, router_id, network_id, None) - contract = self.manager.create_tenant_contract(tenant_id) + # Program APIC + self.manager.add_router_interface(atenant_id, arouter_id, + anetwork_id) - epg = self.manager.ensure_epg_created_for_network(tenant_id, - network_id, - network['name']) - # Delete contract for this epg - self.manager.delete_contract_for_epg(tenant_id, epg.epg_id, - contract.contract_id, - provider=epg.provider) + def remove_router_interface_precommit(self, context, router_id, + interface_info): + if 'subnet_id' in interface_info: + subnet = self.get_subnet(context, interface_info['subnet_id']) + network_id = subnet['network_id'] + tenant_id = subnet['tenant_id'] + else: + port = self.get_port(context, interface_info['port_id']) + network_id = port['network_id'] + tenant_id = port['tenant_id'] + + # Map openstack IDs to APIC IDs + atenant_id, arouter_id, anetwork_id, _ = self._map_names( + context, tenant_id, router_id, network_id, None) + + # Program APIC + self.manager.remove_router_interface(atenant_id, arouter_id, + anetwork_id) + + def delete_router_precommit(self, context, router_id): + context._plugin = self + with apic_mapper.mapper_context(context) as ctx: + arouter_id = router_id and self.name_mapper.router(ctx, router_id) + self.manager.delete_router(arouter_id) + + def update_router_postcommit(self, context, router): + context._plugin = self + with apic_mapper.mapper_context(context) as ctx: + arouter_id = router['id'] and self.name_mapper.router(ctx, + router['id']) + with self.manager.apic.transaction() as trs: + self.manager.create_router(arouter_id, transaction=trs) + if router['admin_state_up']: + self.manager.enable_router(arouter_id, transaction=trs) + else: + self.manager.disable_router(arouter_id, transaction=trs) + + # Router API + + def update_router(self, context, id, router): + result = super(ApicL3ServicePlugin, self).update_router(context, + id, router) + self.update_router_postcommit(context, result) + return result + + def delete_router(self, context, router_id): + self.delete_router_precommit(context, router_id) + result = super(ApicL3ServicePlugin, self).delete_router(context, + router_id) + return result + + # Router Interface API + def add_router_interface(self, context, router_id, interface_info): + # Create interface in parent + result = super(ApicL3ServicePlugin, self).add_router_interface( + context, router_id, interface_info) try: - return super(ApicL3ServicePlugin, self).remove_router_interface( - context, router_id, interface_info) + self.add_router_interface_postcommit(context, router_id, + interface_info) except Exception: with excutils.save_and_reraise_exception(): - self._add_epg_to_contract(tenant_id, epg, contract) + # Rollback db operation + super(ApicL3ServicePlugin, self).remove_router_interface( + context, router_id, interface_info) + return result + + def remove_router_interface(self, context, router_id, interface_info): + self.remove_router_interface_precommit(context, router_id, + interface_info) + return super(ApicL3ServicePlugin, self).remove_router_interface( + context, router_id, interface_info) diff --git a/neutron/tests/unit/services/l3_router/test_l3_apic_plugin.py b/neutron/tests/unit/services/l3_router/test_l3_apic_plugin.py index 8d829a5ea..bc52fa9ea 100644 --- a/neutron/tests/unit/services/l3_router/test_l3_apic_plugin.py +++ b/neutron/tests/unit/services/l3_router/test_l3_apic_plugin.py @@ -21,16 +21,19 @@ import mock sys.modules["apicapi"] = mock.Mock() +from neutron.plugins.ml2.drivers.cisco.apic import mechanism_apic as md from neutron.services.l3_router import l3_apic from neutron.tests.unit.ml2.drivers.cisco.apic import ( test_cisco_apic_common as mocked) from neutron.tests.unit import testlib_api + TENANT = 'tenant1' TENANT_CONTRACT = 'abcd' ROUTER = 'router1' SUBNET = 'subnet1' NETWORK = 'network1' +PORT = 'port1' NETWORK_NAME = 'one_network' NETWORK_EPG = 'one_network-epg' TEST_SEGMENT1 = 'test-segment1' @@ -64,79 +67,71 @@ class FakePort(object): class TestCiscoApicL3Plugin(testlib_api.SqlTestCase, mocked.ControllerMixin, mocked.ConfigMixin): - def setUp(self): super(TestCiscoApicL3Plugin, self).setUp() + mock.patch('neutron.plugins.ml2.drivers.cisco.apic.apic_model.' + 'ApicDbModel').start() mocked.ControllerMixin.set_up_mocks(self) mocked.ConfigMixin.set_up_mocks(self) self.plugin = l3_apic.ApicL3ServicePlugin() + md.APICMechanismDriver.get_router_synchronizer = mock.Mock() self.context = FakeContext() self.context.tenant_id = TENANT - self.interface_info = {'subnet_id': SUBNET, 'network_id': NETWORK, - 'name': NETWORK_NAME} + self.interface_info = {'subnet': {'subnet_id': SUBNET}, + 'port': {'port_id': PORT}} + self.subnet = {'network_id': NETWORK, 'tenant_id': TENANT} + self.port = {'tenant_id': TENANT, + 'network_id': NETWORK, + 'fixed_ips': [{'subnet_id': SUBNET}]} + self.plugin.name_mapper = mock.Mock() + l3_apic.apic_mapper.mapper_context = self.fake_transaction + self.plugin.name_mapper.tenant.return_value = mocked.APIC_TENANT + self.plugin.name_mapper.network.return_value = mocked.APIC_NETWORK + self.plugin.name_mapper.subnet.return_value = mocked.APIC_SUBNET + self.plugin.name_mapper.port.return_value = mocked.APIC_PORT + self.plugin.name_mapper.router.return_value = mocked.APIC_ROUTER + self.plugin.name_mapper.app_profile.return_value = mocked.APIC_AP self.contract = FakeContract() - self.plugin.manager.create_tenant_contract = mock.Mock() - ctmk = mock.PropertyMock(return_value=self.contract.contract_id) - type(self.plugin.manager.create_tenant_contract).contract_id = ctmk - self.epg = FakeEpg() - self.plugin.manager.ensure_epg_created_for_network = mock.Mock() - epmk = mock.PropertyMock(return_value=self.epg.epg_id) - type(self.plugin.manager.ensure_epg_created_for_network).epg_id = epmk - - self.plugin.manager.db.get_provider_contract = mock.Mock( - return_value=None) - self.plugin.manager.set_contract_for_epg = mock.Mock( - return_value=True) - - self.plugin.get_subnet = mock.Mock(return_value=self.interface_info) + self.plugin.get_router = mock.Mock( + return_value={'id': ROUTER, 'admin_state_up': True}) + self.plugin.manager = mock.Mock() + self.plugin.manager.apic.transaction = self.fake_transaction + + self.plugin.get_subnet = mock.Mock(return_value=self.subnet) self.plugin.get_network = mock.Mock(return_value=self.interface_info) - mock.patch('neutron.db.l3_gwmode_db.L3_NAT_db_mixin.' + self.plugin.get_port = mock.Mock(return_value=self.port) + mock.patch('neutron.db.l3_dvr_db.L3_NAT_with_dvr_db_mixin.' '_core_plugin').start() - mock.patch('neutron.db.l3_gwmode_db.L3_NAT_db_mixin.' + mock.patch('neutron.db.l3_dvr_db.L3_NAT_with_dvr_db_mixin.' 'add_router_interface').start() - mock.patch('neutron.db.l3_gwmode_db.L3_NAT_db_mixin.' + mock.patch('neutron.db.l3_dvr_db.L3_NAT_with_dvr_db_mixin.' 'remove_router_interface').start() mock.patch('neutron.openstack.common.excutils.' 'save_and_reraise_exception').start() - def test_add_router_interface(self): + def _test_add_router_interface(self, interface_info): mgr = self.plugin.manager - self.plugin.add_router_interface(self.context, ROUTER, - self.interface_info) - mgr.create_tenant_contract.assert_called_once_with(TENANT) - mgr.create_tenant_contract.assertEqual(TENANT_CONTRACT) - mgr.ensure_epg_created_for_network.assert_called_once_with( - TENANT, NETWORK, NETWORK_NAME) - mgr.ensure_epg_created_for_network.assertEqual(NETWORK_EPG) - mgr.db.get_provider_contract.assert_called_once() - mgr.db.get_provider_contract.assertEqual(None) - mgr.set_contract_for_epg.assert_called_once() - - def test_remove_router_interface(self): + self.plugin.add_router_interface(self.context, ROUTER, interface_info) + mgr.create_router.assert_called_once_with(mocked.APIC_ROUTER, + transaction='transaction') + mgr.add_router_interface.assert_called_once_with( + mocked.APIC_TENANT, mocked.APIC_ROUTER, mocked.APIC_NETWORK) + + def _test_remove_router_interface(self, interface_info): mgr = self.plugin.manager self.plugin.remove_router_interface(self.context, ROUTER, - self.interface_info) - mgr.create_tenant_contract.assert_called_once_with(TENANT) - mgr.ensure_epg_created_for_network.assert_called_once_with( - TENANT, NETWORK, NETWORK_NAME) - mgr.ensure_epg_created_for_network.assertEqual(NETWORK_EPG) + interface_info) mgr.delete_contract_for_epg.assert_called_once() - def test_add_router_interface_fail_contract_delete(self): - mgr = self.plugin.manager - with mock.patch('neutron.db.l3_gwmode_db.L3_NAT_db_mixin.' - 'add_router_interface', - side_effect=KeyError()): - self.plugin.add_router_interface(self.context, ROUTER, - self.interface_info) - mgr.delete_contract_for_epg.assert_called_once() - - def test_delete_router_interface_fail_contract_create(self): - mgr = self.plugin.manager - with mock.patch('neutron.db.l3_gwmode_db.L3_NAT_db_mixin.' - 'remove_router_interface', - side_effect=KeyError()): - self.plugin.remove_router_interface(self.context, ROUTER, - self.interface_info) - mgr.set_contract_for_epg.assert_called_once() + def test_add_router_interface_subnet(self): + self._test_add_router_interface(self.interface_info['subnet']) + + def test_add_router_interface_port(self): + self._test_add_router_interface(self.interface_info['port']) + + def test_remove_router_interface_subnet(self): + self._test_remove_router_interface(self.interface_info['subnet']) + + def test_remove_router_interface_port(self): + self._test_remove_router_interface(self.interface_info['port'])