diff --git a/gbpservice/neutron/plugins/ml2plus/drivers/apic_aim/db.py b/gbpservice/neutron/plugins/ml2plus/drivers/apic_aim/db.py index 41076d285..1f9a89481 100644 --- a/gbpservice/neutron/plugins/ml2plus/drivers/apic_aim/db.py +++ b/gbpservice/neutron/plugins/ml2plus/drivers/apic_aim/db.py @@ -94,6 +94,17 @@ class DbMixin(object): vrf_name=vrf.name). all()) + def _get_address_scopes_owning_vrf(self, session, vrf): + return (session.query(as_db.AddressScope). + join(AddressScopeMapping, + AddressScopeMapping.scope_id == as_db.AddressScope.id). + filter(AddressScopeMapping.vrf_tenant_name == + vrf.tenant_name, + AddressScopeMapping.vrf_name == vrf.name, + AddressScopeMapping.vrf_owned). + order_by(as_db.AddressScope.ip_version). + all()) + def _get_address_scope_vrf(self, mapping): return aim_resource.VRF( tenant_name=mapping.vrf_tenant_name, diff --git a/gbpservice/neutron/plugins/ml2plus/drivers/apic_aim/mechanism_driver.py b/gbpservice/neutron/plugins/ml2plus/drivers/apic_aim/mechanism_driver.py index 4517f51c5..50e79f69d 100644 --- a/gbpservice/neutron/plugins/ml2plus/drivers/apic_aim/mechanism_driver.py +++ b/gbpservice/neutron/plugins/ml2plus/drivers/apic_aim/mechanism_driver.py @@ -1091,6 +1091,8 @@ class ApicMechanismDriver(api_plus.MechanismDriver, mapping = self._get_address_scope_mapping(session, id) if mapping: vrf = self._get_address_scope_vrf(mapping) + scopes = self._get_address_scopes_owning_vrf(session, vrf) + self._update_vrf_display_name(aim_ctx, vrf, scopes) else: dname = aim_utils.sanitize_display_name(current['name']) vrf = self._map_address_scope(session, current) @@ -1114,9 +1116,9 @@ class ApicMechanismDriver(api_plus.MechanismDriver, mapping = self._get_address_scope_mapping(session, current['id']) if current['name'] != original['name'] and mapping.vrf_owned: - dname = aim_utils.sanitize_display_name(current['name']) vrf = self._get_address_scope_vrf(mapping) - self.aim.update(aim_ctx, vrf, display_name=dname) + scopes = self._get_address_scopes_owning_vrf(session, vrf) + self._update_vrf_display_name(aim_ctx, vrf, scopes) def delete_address_scope_precommit(self, context): current = context.current @@ -1128,10 +1130,11 @@ class ApicMechanismDriver(api_plus.MechanismDriver, if mapping.vrf_owned: vrf = self._get_address_scope_vrf(mapping) - mappings = self._get_address_scope_mappings_for_vrf(session, vrf) - if len(mappings) == 1: + session.delete(mapping) + scopes = self._get_address_scopes_owning_vrf(session, vrf) + self._update_vrf_display_name(aim_ctx, vrf, scopes) + if not scopes: self.aim.delete(aim_ctx, vrf) - session.delete(mapping) def extend_address_scope_dict(self, session, scope, result): if result.get(api_plus.BULK_EXTENDED): @@ -1153,6 +1156,20 @@ class ApicMechanismDriver(api_plus.MechanismDriver, result[cisco_apic.DIST_NAMES] = dist_names result[cisco_apic.SYNC_STATE] = sync_state + def _update_vrf_display_name(self, aim_ctx, vrf, scopes): + # Assumes scopes is sorted by ip_version. + if not scopes: + return + elif (len(scopes) == 1 or not scopes[1].name or + scopes[0].name == scopes[1].name): + dname = scopes[0].name + elif not scopes[0].name: + dname = scopes[1].name + else: + dname = scopes[0].name + '-' + scopes[1].name + dname = aim_utils.sanitize_display_name(dname) + self.aim.update(aim_ctx, vrf, display_name=dname) + def create_router(self, context, current): LOG.debug("APIC AIM MD creating router: %s", current) @@ -4110,10 +4127,11 @@ class ApicMechanismDriver(api_plus.MechanismDriver, mgr.expected_aim_ctx) def _validate_address_scopes(self, mgr): + owned_scopes_by_vrf = defaultdict(list) for scope_db in mgr.actual_session.query(as_db.AddressScope): self._expect_project(mgr, scope_db.project_id) mapping = scope_db.aim_mapping - if mapping and not mapping.vrf_owned: + if mapping: mgr.expect_db_instance(mapping) else: vrf = self._map_address_scope(mgr.expected_session, scope_db) @@ -4121,13 +4139,19 @@ class ApicMechanismDriver(api_plus.MechanismDriver, mgr.expected_session, scope_db.id, vrf) vrf = self._get_address_scope_vrf(mapping) vrf.monitored = not mapping.vrf_owned - # REVISIT: Need deterministic display_name for isomorphic - # address scopes that own VRF. vrf.display_name = ( aim_utils.sanitize_display_name(scope_db.name) if mapping.vrf_owned else "") vrf.policy_enforcement_pref = 'enforced' - mgr.expect_aim_resource(vrf) + mgr.expect_aim_resource(vrf, replace=True) + if mapping.vrf_owned: + scopes = owned_scopes_by_vrf[tuple(vrf.identity)] + scopes.append(scope_db) + # REVISIT: Fail if multiple scopes for same address family? + if len(scopes) > 1: + scopes = sorted(scopes, key=lambda scope: scope.ip_version) + self._update_vrf_display_name( + mgr.expected_aim_ctx, vrf, scopes) def _validate_routers(self, mgr): router_dbs = {} diff --git a/gbpservice/neutron/tests/unit/plugins/ml2plus/test_apic_aim.py b/gbpservice/neutron/tests/unit/plugins/ml2plus/test_apic_aim.py index d13eaa681..71fae29b9 100644 --- a/gbpservice/neutron/tests/unit/plugins/ml2plus/test_apic_aim.py +++ b/gbpservice/neutron/tests/unit/plugins/ml2plus/test_apic_aim.py @@ -336,12 +336,19 @@ class ApicAimTestCase(test_address_scope.AddressScopeTestCase, resource = self._find_by_dn(dn, cls) self.assertIsNotNone(resource) - def _check_dn(self, resource, aim_resource, key): + def _check_dn(self, resource, aim_resource, key, dname=None): dist_names = resource.get('apic:distinguished_names') self.assertIsInstance(dist_names, dict) dn = dist_names.get(key) self.assertIsInstance(dn, six.string_types) self.assertEqual(aim_resource.dn, dn) + aim_resource = self._find_by_dn(dn, aim_resource.__class__) + self.assertIsNotNone(aim_resource) + if dname is None: + name = resource.get('name') + self.assertIsInstance(name, six.string_types) + dname = aim_utils.sanitize_display_name(name) + self.assertEqual(dname, aim_resource.display_name) def _check_no_dn(self, resource, key): dist_names = resource.get('apic:distinguished_names') @@ -463,6 +470,7 @@ class TestAimMapping(ApicAimTestCase): self.mock_ns = self.call_wrapper.setUp( nat_strategy.DistributedNatStrategy) self._actual_scopes = {} + self._scope_vrf_dnames = {} super(TestAimMapping, self).setUp() def tearDown(self): @@ -647,7 +655,8 @@ class TestAimMapping(ApicAimTestCase): elif scope: scope = self._actual_scopes.get(scope['id'], scope) vrf_aname = self.name_mapper.address_scope(None, scope['id']) - vrf_dname = scope['name'] + vrf_dname = self._scope_vrf_dnames.get( + scope['id'], scope['name']) vrf_project = scope['tenant_id'] vrf_tenant_aname = self.name_mapper.project(None, vrf_project) tenant_aname = vrf_tenant_aname @@ -777,6 +786,8 @@ class TestAimMapping(ApicAimTestCase): dns = copy.copy(scope.get(DN)) actual_scope = self._actual_scopes.get(scope['id'], scope) + dname = self._scope_vrf_dnames.get( + actual_scope['id'], actual_scope['name']) tenant_aname = self.name_mapper.project( None, actual_scope['tenant_id']) @@ -787,7 +798,7 @@ class TestAimMapping(ApicAimTestCase): aim_vrf = self._get_vrf(aname, tenant_aname) self.assertEqual(tenant_aname, aim_vrf.tenant_name) self.assertEqual(aname, aim_vrf.name) - self.assertEqual(actual_scope['name'], aim_vrf.display_name) + self.assertEqual(dname, aim_vrf.display_name) self.assertEqual('enforced', aim_vrf.policy_enforcement_pref) self._check_dn_is_resource(dns, 'VRF', aim_vrf) @@ -869,10 +880,12 @@ class TestAimMapping(ApicAimTestCase): for scope in scopes or []: actual_scope = self._actual_scopes.get(scope['id'], scope) + dname = self._scope_vrf_dnames.get( + actual_scope['id'], actual_scope['name']) self._check_router_vrf( self.name_mapper.address_scope(None, actual_scope['id']), - actual_scope['name'], actual_scope['tenant_id'], - dns, 'as_%s-VRF' % scope['id']) + dname, actual_scope['tenant_id'], dns, + 'as_%s-VRF' % scope['id']) # The AIM Subnets are validated in _check_subnet, so just # check that their DNs are present and valid. @@ -1755,6 +1768,8 @@ class TestAimMapping(ApicAimTestCase): scope46i_vrf, 4, name='as4i')['address_scope'] scope4i_id = scope4i['id'] self._actual_scopes[scope4i_id] = scope6 + self._scope_vrf_dnames[scope4i_id] = 'as4i-as6' + self._scope_vrf_dnames[scope6_id] = 'as4i-as6' self._check_address_scope(scope4i) pool4i = self._make_subnetpool( self.fmt, ['10.1.0.0/16'], name='sp4i', tenant_id=self._tenant_id, @@ -4849,19 +4864,36 @@ class TestExtensionAttributes(ApicAimTestCase): def test_isomorphic_address_scopes_lifecycle(self): # Create initial v4 scope. scope4 = self._make_address_scope( - self.fmt, 4, name='as')['address_scope'] + self.fmt, 4, name='as4')['address_scope'] dn = scope4['apic:distinguished_names']['VRF'] + vrf = self._find_by_dn(dn, aim_resource.VRF) + self._check_dn(scope4, vrf, 'VRF', 'as4') # Create isomorphic v6 scope, using v4 scope's pre-existing # DN. scope6 = self._make_address_scope_for_vrf( - dn, n_constants.IP_VERSION_6)['address_scope'] + dn, n_constants.IP_VERSION_6, name='as6')['address_scope'] self.assertEqual(dn, scope6['apic:distinguished_names']['VRF']) + self._check_dn(scope6, vrf, 'VRF', 'as4-as6') + + # Update v4 scope name. + data = {'address_scope': {'name': ''}} + self._update('address-scopes', scope4['id'], data) + self._check_dn(scope4, vrf, 'VRF', 'as6') + + # Update v4 scope name again. + data = {'address_scope': {'name': 'x4'}} + self._update('address-scopes', scope4['id'], data) + self._check_dn(scope4, vrf, 'VRF', 'x4-as6') + + # Update v6 scope name. + data = {'address_scope': {'name': 'x6'}} + self._update('address-scopes', scope6['id'], data) + self._check_dn(scope6, vrf, 'VRF', 'x4-x6') # Delete v4 scope. self._delete('address-scopes', scope4['id']) - vrf = self._find_by_dn(dn, aim_resource.VRF) - self.assertIsNotNone(vrf) + self._check_dn(scope6, vrf, 'VRF', 'x6') # Delete v6 scope. self._delete('address-scopes', scope6['id']) @@ -4872,17 +4904,24 @@ class TestExtensionAttributes(ApicAimTestCase): scope4 = self._make_address_scope( self.fmt, 4, name='as')['address_scope'] dn = scope4['apic:distinguished_names']['VRF'] + vrf = self._find_by_dn(dn, aim_resource.VRF) + self._check_dn(scope4, vrf, 'VRF', 'as') # Create isomorphic v6 scope, using v4 scope's pre-existing - # DN. + # DN, with same name. scope6 = self._make_address_scope_for_vrf( - dn, n_constants.IP_VERSION_6)['address_scope'] + dn, n_constants.IP_VERSION_6, name='as')['address_scope'] self.assertEqual(dn, scope6['apic:distinguished_names']['VRF']) + self._check_dn(scope6, vrf, 'VRF', 'as') + + # Update v4 scope name to not match. + data = {'address_scope': {'name': 'as4'}} + self._update('address-scopes', scope4['id'], data) + self._check_dn(scope4, vrf, 'VRF', 'as4-as') # Delete v6 scope. self._delete('address-scopes', scope6['id']) - vrf = self._find_by_dn(dn, aim_resource.VRF) - self.assertIsNotNone(vrf) + self._check_dn(scope4, vrf, 'VRF', 'as4') # Delete v4 scope. self._delete('address-scopes', scope4['id']) @@ -4999,14 +5038,14 @@ class TestExternalConnectivityBase(object): ext_bd = aim_resource.BridgeDomain( tenant_name=self.t1_aname, name='EXT-l1') ext_vrf = aim_resource.VRF(tenant_name=self.t1_aname, name='EXT-l1') - self._check_dn(net1, ext_epg, 'EndpointGroup') - self._check_dn(net1, ext_bd, 'BridgeDomain') - self._check_dn(net1, ext_vrf, 'VRF') + self._check_dn(net1, ext_epg, 'EndpointGroup', 'EXT-l1') + self._check_dn(net1, ext_bd, 'BridgeDomain', 'EXT-l1') + self._check_dn(net1, ext_vrf, 'VRF', 'EXT-l1') net1 = self._show('networks', net1['id'])['network'] - self._check_dn(net1, ext_epg, 'EndpointGroup') - self._check_dn(net1, ext_bd, 'BridgeDomain') - self._check_dn(net1, ext_vrf, 'VRF') + self._check_dn(net1, ext_epg, 'EndpointGroup', 'EXT-l1') + self._check_dn(net1, ext_bd, 'BridgeDomain', 'EXT-l1') + self._check_dn(net1, ext_vrf, 'VRF', 'EXT-l1') self._validate() # test no-op CIDR update diff --git a/gbpservice/neutron/tests/unit/services/grouppolicy/test_aim_validation.py b/gbpservice/neutron/tests/unit/services/grouppolicy/test_aim_validation.py index 7af8b48ec..db906e255 100644 --- a/gbpservice/neutron/tests/unit/services/grouppolicy/test_aim_validation.py +++ b/gbpservice/neutron/tests/unit/services/grouppolicy/test_aim_validation.py @@ -22,6 +22,7 @@ from aim import context as aim_context from neutron.db.models import segment from neutron.plugins.ml2 import models as ml2_models from neutron.tests.unit.extensions import test_securitygroup +from neutron_lib import constants as n_constants from neutron_lib import context as n_context from oslo_config import cfg @@ -76,11 +77,17 @@ class AimValidationTestMixin(object): # Delete the AIM resource and test. self.aim_mgr.delete(self.aim_ctx, resource) self._validate_repair_validate() + self.assertTrue( + actual_resource.user_equal( + self.aim_mgr.get(self.aim_ctx, resource))) # Modify the AIM resource and test. self.aim_mgr.update( self.aim_ctx, resource, display_name='not what it was') self._validate_repair_validate() + self.assertTrue( + actual_resource.user_equal( + self.aim_mgr.get(self.aim_ctx, resource))) # Add unexpected AIM resource and test. setattr(resource, unexpected_attr_name, unexpected_attr_value) @@ -270,15 +277,15 @@ class TestNeutronMapping(AimValidationTestCase): def test_address_scope(self): # Create address scope. - scope = self._make_address_scope( - self.fmt, 4, name='as1')['address_scope'] - scope_id = scope['id'] - vrf_dn = scope['apic:distinguished_names']['VRF'] + scope4 = self._make_address_scope( + self.fmt, 4, name='as4')['address_scope'] + scope4_id = scope4['id'] + vrf_dn = scope4['apic:distinguished_names']['VRF'] self._validate() # Delete the address scope's mapping record and test. (self.db_session.query(db.AddressScopeMapping). - filter_by(scope_id=scope_id). + filter_by(scope_id=scope4_id). delete()) self._validate_repair_validate() @@ -286,7 +293,47 @@ class TestNeutronMapping(AimValidationTestCase): vrf = aim_resource.VRF.from_dn(vrf_dn) self._test_aim_resource(vrf) - # REVISIT: Test isomorphic address scopes. + # Create isomorphic v6 address scope. + scope6 = self._make_address_scope_for_vrf( + vrf_dn, n_constants.IP_VERSION_6, name='as6')['address_scope'] + scope6_id = scope6['id'] + self.assertEqual(vrf_dn, scope6['apic:distinguished_names']['VRF']) + self._validate() + + # Test AIM VRF. + self._test_aim_resource(vrf) + + # Delete the initial address scope's mapping record and test. + (self.db_session.query(db.AddressScopeMapping). + filter_by(scope_id=scope4_id). + delete()) + self._validate_repair_validate() + scope4 = self._show('address-scopes', scope4_id)['address_scope'] + self.assertEqual(vrf_dn, scope4['apic:distinguished_names']['VRF']) + scope6 = self._show('address-scopes', scope6_id)['address_scope'] + self.assertEqual(vrf_dn, scope6['apic:distinguished_names']['VRF']) + + # Test AIM VRF. + self._test_aim_resource(vrf) + + # Delete the 2nd address scope's mapping record and + # test. Without this record, there is no way to know that the + # scopes were previously isomorphic, so they no longer will + # be isomorphic after repair. + (self.db_session.query(db.AddressScopeMapping). + filter_by(scope_id=scope6_id). + delete()) + self._validate_repair_validate() + scope4 = self._show('address-scopes', scope4_id)['address_scope'] + self.assertEqual(vrf_dn, scope4['apic:distinguished_names']['VRF']) + scope6 = self._show('address-scopes', scope6_id)['address_scope'] + scope6_vrf_dn = scope6['apic:distinguished_names']['VRF'] + self.assertNotEqual(vrf_dn, scope6_vrf_dn) + + # Test both AIM VRFs. + self._test_aim_resource(vrf) + scope6_vrf = aim_resource.VRF.from_dn(scope6_vrf_dn) + self._test_aim_resource(scope6_vrf) def _test_network_attrs(self, original): current = self._show('networks', original['id'])['network']