From f534f36246fd0b41bcdc2a664369507f9e299266 Mon Sep 17 00:00:00 2001 From: Boris Bobrov Date: Fri, 8 Jul 2016 02:49:03 +0300 Subject: [PATCH] Faster id mapping lookup id_mapping_api was designed to make a query per entity to fetch public ids. This lead to a very poor performance when there were many entries in LDAP. For example, for 15k entries 15k MySQL queries were required. For the first run 15k INSERTs were required, which makes things even worse. Change this behavior to fetch related mappings from MySQL as a list and perform the necessary join in-memory. bp ldap-preprocessing Partial-Bug: 1582585 Change-Id: I2c266e91f2f05be760f8a3eea3738868243cc9c6 --- keystone/identity/core.py | 74 ++++++++++++++++--- keystone/identity/mapping_backends/base.py | 9 +++ keystone/identity/mapping_backends/sql.py | 4 + .../tests/unit/test_backend_id_mapping_sql.py | 42 +++++++++++ keystone/tests/unit/test_backend_ldap.py | 17 +++++ .../domain_mapping_list-a368ac5a252ec84f.yaml | 6 ++ 6 files changed, 142 insertions(+), 10 deletions(-) create mode 100644 releasenotes/notes/domain_mapping_list-a368ac5a252ec84f.yaml diff --git a/keystone/identity/core.py b/keystone/identity/core.py index 8170950720..cdcb702eee 100644 --- a/keystone/identity/core.py +++ b/keystone/identity/core.py @@ -573,8 +573,8 @@ class Manager(manager.Manager): return self._set_domain_id_and_mapping_for_single_ref( ref, domain_id, driver, entity_type, conf) elif isinstance(ref, list): - return [self._set_domain_id_and_mapping( - x, domain_id, driver, entity_type) for x in ref] + return self._set_domain_id_and_mapping_for_list( + ref, domain_id, driver, entity_type, conf) else: raise ValueError(_('Expected dict or list: %s') % type(ref)) @@ -583,6 +583,16 @@ class Manager(manager.Manager): return (driver is not self.driver or not driver.generates_uuids() or not driver.is_domain_aware()) + def _insert_new_public_id(self, local_entity, ref, driver): + # Need to create a mapping. If the driver generates UUIDs + # then pass the local UUID in as the public ID to use. + public_id = None + if driver.generates_uuids(): + public_id = ref['id'] + ref['id'] = self.id_mapping_api.create_id_mapping( + local_entity, public_id) + LOG.debug('Created new mapping to public ID: %s', ref['id']) + def _set_domain_id_and_mapping_for_single_ref(self, ref, domain_id, driver, entity_type, conf): LOG.debug('Local ID: %s', ref['id']) @@ -600,16 +610,60 @@ class Manager(manager.Manager): LOG.debug('Found existing mapping to public ID: %s', ref['id']) else: - # Need to create a mapping. If the driver generates UUIDs - # then pass the local UUID in as the public ID to use. - if driver.generates_uuids(): - public_id = ref['id'] - ref['id'] = self.id_mapping_api.create_id_mapping( - local_entity, public_id) - LOG.debug('Created new mapping to public ID: %s', - ref['id']) + self._insert_new_public_id(local_entity, ref, driver) return ref + def _set_domain_id_and_mapping_for_list(self, ref_list, domain_id, driver, + entity_type, conf): + """Set domain id and mapping for a list of refs. + + The method modifies refs in-place. + """ + if not ref_list: + return [] + + for r in ref_list: + self._insert_domain_id_if_needed(r, driver, domain_id, conf) + + if not self._is_mapping_needed(driver): + return ref_list + + # build a map of refs for fast look-up + refs_map = {} + for r in ref_list: + refs_map[(r['id'], entity_type, r['domain_id'])] = r + + # NOTE(breton): there are cases when the driver is not domain aware and + # no domain_id was explicitely provided for list operation. domain_id + # gets inserted into refs, but not passed into this method. Lets use + # domain_id from one of the refs. + if not domain_id: + domain_id = ref_list[0]['domain_id'] + + # fetch all mappings for the domain, lookup the user at the map built + # at previous step and replace his id. + domain_mappings = self.id_mapping_api.get_domain_mapping_list( + domain_id) + for _mapping in domain_mappings: + idx = (_mapping.local_id, _mapping.entity_type, _mapping.domain_id) + try: + ref = refs_map.pop(idx) + # due to python specifics, `ref` still points to an item in + # `ref_list`. That's why when we change it here, it gets + # changed in `ref_list`. + ref['id'] = _mapping.public_id + except KeyError: + pass # some old entry, skip it + + # at this point, all known refs were granted a public_id. For the refs + # left, there are no mappings. They need to be created. + for ref in refs_map.values(): + local_entity = {'domain_id': ref['domain_id'], + 'local_id': ref['id'], + 'entity_type': entity_type} + self._insert_new_public_id(local_entity, ref, driver) + return ref_list + def _insert_domain_id_if_needed(self, ref, driver, domain_id, conf): """Insert the domain ID into the ref, if required. diff --git a/keystone/identity/mapping_backends/base.py b/keystone/identity/mapping_backends/base.py index abf09d5676..1d15502c30 100644 --- a/keystone/identity/mapping_backends/base.py +++ b/keystone/identity/mapping_backends/base.py @@ -34,6 +34,15 @@ class MappingDriverV8(object): """ raise exception.NotImplemented() # pragma: no cover + @abc.abstractmethod + def get_domain_mapping_list(self, domain_id): + """Return mappings for the domain. + + :param domain_id: Domain ID to get mappings for. + :returns: list of mappings. + """ + raise exception.NotImplemented() # pragma: no cover + @abc.abstractmethod def get_id_mapping(self, public_id): """Return the local mapping. diff --git a/keystone/identity/mapping_backends/sql.py b/keystone/identity/mapping_backends/sql.py index 7da264dd9e..0ec9be0f51 100644 --- a/keystone/identity/mapping_backends/sql.py +++ b/keystone/identity/mapping_backends/sql.py @@ -57,6 +57,10 @@ class Mapping(base.MappingDriverV8): except sql.NotFound: return None + def get_domain_mapping_list(self, domain_id): + with sql.session_for_read() as session: + return session.query(IDMapping).filter_by(domain_id=domain_id) + def get_id_mapping(self, public_id): with sql.session_for_read() as session: mapping_ref = session.query(IDMapping).get(public_id) diff --git a/keystone/tests/unit/test_backend_id_mapping_sql.py b/keystone/tests/unit/test_backend_id_mapping_sql.py index da8bd3176d..74b9b55c00 100644 --- a/keystone/tests/unit/test_backend_id_mapping_sql.py +++ b/keystone/tests/unit/test_backend_id_mapping_sql.py @@ -290,3 +290,45 @@ class SqlIDMapping(test_backend_sql.SqlTests): # Purge mappings the remaining mappings self.id_mapping_api.purge_mappings({}) self.assertIsNone(self.id_mapping_api.get_public_id(local_entity5)) + + def test_get_domain_mapping_list(self): + local_id1 = uuid.uuid4().hex + local_id2 = uuid.uuid4().hex + local_id3 = uuid.uuid4().hex + local_id4 = uuid.uuid4().hex + local_id5 = uuid.uuid4().hex + + # Create five mappings,two in domainA, three in domainB + local_entity1 = {'domain_id': self.domainA['id'], + 'local_id': local_id1, + 'entity_type': mapping.EntityType.USER} + local_entity2 = {'domain_id': self.domainA['id'], + 'local_id': local_id2, + 'entity_type': mapping.EntityType.USER} + local_entity3 = {'domain_id': self.domainB['id'], + 'local_id': local_id3, + 'entity_type': mapping.EntityType.GROUP} + local_entity4 = {'domain_id': self.domainB['id'], + 'local_id': local_id4, + 'entity_type': mapping.EntityType.USER} + local_entity5 = {'domain_id': self.domainB['id'], + 'local_id': local_id5, + 'entity_type': mapping.EntityType.USER} + + local_entity1['public_id'] = self.id_mapping_api.create_id_mapping( + local_entity1) + local_entity2['public_id'] = self.id_mapping_api.create_id_mapping( + local_entity2) + local_entity3['public_id'] = self.id_mapping_api.create_id_mapping( + local_entity3) + local_entity4['public_id'] = self.id_mapping_api.create_id_mapping( + local_entity4) + local_entity5['public_id'] = self.id_mapping_api.create_id_mapping( + local_entity5) + + # list mappings for domainA + domain_a_mappings = self.id_mapping_api.get_domain_mapping_list( + self.domainA['id']) + domain_a_mappings = [m.to_dict() for m in domain_a_mappings] + self.assertItemsEqual([local_entity1, local_entity2], + domain_a_mappings) diff --git a/keystone/tests/unit/test_backend_ldap.py b/keystone/tests/unit/test_backend_ldap.py index 9e8ab38c50..40e4182050 100644 --- a/keystone/tests/unit/test_backend_ldap.py +++ b/keystone/tests/unit/test_backend_ldap.py @@ -3121,6 +3121,23 @@ class DomainSpecificLDAPandSQLIdentity( domain_scope=self.domains['domain1']['id']), matchers.HasLength(1)) + def test_get_domain_mapping_list_is_used(self): + # before get_domain_mapping_list was introduced, it was required to + # make N calls to the database for N users, and it was slow. + # get_domain_mapping_list solves this problem and should be used + # when multiple users are fetched from domain-specific backend. + for i in range(5): + unit.create_user(self.identity_api, + domain_id=self.domains['domain1']['id']) + + with mock.patch.multiple(self.id_mapping_api, + get_domain_mapping_list=mock.DEFAULT, + get_id_mapping=mock.DEFAULT) as mocked: + self.identity_api.list_users( + domain_scope=self.domains['domain1']['id']) + mocked['get_domain_mapping_list'].assert_called() + mocked['get_id_mapping'].assert_not_called() + def test_user_id_comma(self): self.skip_test_overrides('Only valid if it is guaranteed to be ' 'talking to the fakeldap backend') diff --git a/releasenotes/notes/domain_mapping_list-a368ac5a252ec84f.yaml b/releasenotes/notes/domain_mapping_list-a368ac5a252ec84f.yaml new file mode 100644 index 0000000000..0390ad2a02 --- /dev/null +++ b/releasenotes/notes/domain_mapping_list-a368ac5a252ec84f.yaml @@ -0,0 +1,6 @@ +--- +upgrade: + - ID Mapping driver interface has changed. A new method + ``get_domain_mapping_list`` was added for fetching mapping list + for a domain. If you have a custom implementation for the identity + driver, you will need to implement this new method.