]> review.fuel-infra Code Review - openstack-build/neutron-build.git/commitdiff
Apic drivers enhancements (second approach): L3 refactor
authorIvar Lazzaro <ivarlazzaro@gmail.com>
Fri, 22 Aug 2014 01:35:02 +0000 (18:35 -0700)
committerIvar Lazzaro <ivarlazzaro@gmail.com>
Sun, 31 Aug 2014 23:36:16 +0000 (16:36 -0700)
- refactor to leverage Client's transactional capabilities
- General refactor to improve the driver's reliability

Implements blueprint: apic-driver-enhancements

Change-Id: I5a19039e82988a0570622bc1ddb1429e9833d478

neutron/db/migration/alembic_migrations/versions/86d6d9776e2b_cisco_apic_driver_update_l3.py [new file with mode: 0644]
neutron/db/migration/alembic_migrations/versions/HEAD
neutron/plugins/ml2/drivers/cisco/apic/apic_model.py
neutron/services/l3_router/l3_apic.py
neutron/tests/unit/services/l3_router/test_l3_apic_plugin.py

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 (file)
index 0000000..d753b33
--- /dev/null
@@ -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'))
index dbff3d53e07ff422e332dc0e218cd9bce39223fb..afbbf75d328e772531a3b1eaac423c31d31bb29c 100644 (file)
@@ -1 +1 @@
-236b90af57ab
+86d6d9776e2b
index 9111b1546145d493dfda3c90875eebf52c528918..256369f252c73af22683f6dba30ab20392b49f7c 100644 (file)
@@ -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)
index 75622fbc70e6753ebebee353940fb039bcdb144e..f55001bb91b3a10d1d5dbf4804a37129f5958cf4 100644 (file)
 #    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)
index 8d829a5ea918cd707a47ae4b0da94aa5146a81b0..bc52fa9ea083d94c23824695fd7c346a7c89be77 100644 (file)
@@ -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'])