On 04/08/2015 10:21 AM, Jan Cholasta wrote:
Hi,

Dne 31.3.2015 v 12:11 Petr Vobornik napsal(a):
the old implementation tried to get all entries which are member of
group. That means also user. User can't have any members therefore this
costly processing was unnecessary.

New implementation reduces the search only to entries which can have
entries.

Also page size was removed to avoid paging by small pages(default size:
100) which is very slow for many members.

https://fedorahosted.org/freeipa/ticket/4947

Useful to test with #809

1) To search for entries with members, you should search for entries
with the member attribute set ('(member=*)'), not for entries with some
arbitrary object class.

Replaced, new presence index added



2) I don't like how the search in get_memberindirect is limited to an
arbitrary hard-coded subtree. You should go through the object's
attribute_members to figure out which subtrees to search.


The subtree search was removed.


3) Since memberindirect and memberofindirect are not real attributes,
you must define their syntax in ipaldap before you cat set them using
.raw[], otherwise they will be decoded to wrong type.

Added.


4) The processing of memberof should be done even when memberofindirect
is not requested, otherwise its value will depend on whether
memberofindirect was requested or not.

True, but it's the same behavior as before. Could be changed in other patch.



5) I would prefer if all membership processing
(.convert_attribute_members() and .get_indirect_members()) was done in a
single LDAPObject method.

Now, as before, get_indirect_members is called before post callbacks and convert_attribute_members after. If it should be combined, it should be done separately.



Honza

--
Petr Vobornik
From 27dfa6d0e2a1815d496f47b4f10b5a6307f51fda Mon Sep 17 00:00:00 2001
From: Petr Vobornik <pvobo...@redhat.com>
Date: Tue, 31 Mar 2015 10:59:37 +0200
Subject: [PATCH] speed up indirect member processing

the old implementation tried to get all entries which are member of group.
That means also user. User can't have any members therefore this costly
processing was unnecessary.

New implementation reduces the search only to entries which have members.

Also page size was removed to avoid paging by small pages(default size: 100)
which is very slow for many members.

https://fedorahosted.org/freeipa/ticket/4947
---
 install/updates/20-indices.update |  2 +-
 ipalib/plugins/baseldap.py        | 72 +++++++++++++++++++++++++++++++
 ipalib/plugins/host.py            |  2 +-
 ipalib/plugins/role.py            |  8 ++--
 ipapython/ipaldap.py              |  2 +
 ipaserver/plugins/ldap2.py        | 90 ---------------------------------------
 6 files changed, 81 insertions(+), 95 deletions(-)

diff --git a/install/updates/20-indices.update b/install/updates/20-indices.update
index a8a432d9c28a7fb4ca74582e36d4c39fd98df2cf..a9ec9f9eb9bcc228dcbb7eba99879ce5a8251e80 100644
--- a/install/updates/20-indices.update
+++ b/install/updates/20-indices.update
@@ -27,7 +27,7 @@ default:nsSystemIndex: false
 only:nsIndexType: eq,pres,sub
 
 dn: cn=member,cn=index,cn=userRoot,cn=ldbm database,cn=plugins,cn=config
-only:nsIndexType: eq,sub
+only:nsIndexType: eq,pres,sub
 
 dn: cn=uniquemember,cn=index,cn=userRoot,cn=ldbm database,cn=plugins,cn=config
 only:nsIndexType: eq,sub
diff --git a/ipalib/plugins/baseldap.py b/ipalib/plugins/baseldap.py
index c9f98ed12d3a584597af9b0be08361bceca620e7..9c75bc048283546902a89825ba489d42b26f10fd 100644
--- a/ipalib/plugins/baseldap.py
+++ b/ipalib/plugins/baseldap.py
@@ -663,6 +663,61 @@ class LDAPObject(Object):
                         new_attr.append(new_value)
                         break
 
+    def get_memberindirect(self, group_entry):
+        """
+        Get indirect members
+        """
+
+        mo_filter = self.backend.make_filter({'memberof': group_entry.dn})
+        filter = self.backend.combine_filters(
+            ('(member=*)', mo_filter), self.backend.MATCH_ALL)
+        try:
+            result, truncated = self.backend.find_entries(
+                base_dn=self.api.env.basedn,
+                filter=filter,
+                attrs_list=['member'],
+                size_limit=-1, # paged search will get everything anyway
+                paged_search=True)
+            if truncated:
+                raise errors.LimitsExceeded()
+        except errors.NotFound:
+            result = []
+
+        indirect = set()
+        for entry in result:
+            indirect.update(entry.raw.get('member', []))
+        indirect.difference_update(group_entry.raw.get('member', []))
+
+        if indirect:
+            group_entry.raw['memberindirect'] = list(indirect)
+
+    def get_memberofindirect(self, entry):
+
+        dn = entry.dn
+        filter = self.backend.make_filter(
+            {'member': dn, 'memberuser': dn, 'memberhost': dn})
+        try:
+            result, truncated = self.backend.find_entries(
+                base_dn=self.api.env.basedn,
+                filter=filter,
+                attrs_list=[''])
+            if truncated:
+                raise errors.LimitsExceeded()
+        except errors.NotFound:
+            result = []
+
+        direct = set()
+        indirect = set(entry.raw.get('memberof', []))
+        for group_entry in result:
+            dn = str(group_entry.dn)
+            if dn in indirect:
+                indirect.remove(dn)
+                direct.add(dn)
+
+        entry.raw['memberof'] = list(direct)
+        if indirect:
+            entry.raw['memberofindirect'] = list(indirect)
+
     def get_password_attributes(self, ldap, dn, entry_attrs):
         """
         Search on the entry to determine if it has a password or
@@ -1106,6 +1161,12 @@ last, after all sets and adds."""),
                     )
                     break
 
+    def get_indirect_members(self, entry_attrs, attrs_list):
+        if 'memberindirect' in attrs_list:
+            self.obj.get_memberindirect(entry_attrs)
+        if 'memberofindirect' in attrs_list:
+            self.obj.get_memberofindirect(entry_attrs)
+
 class LDAPCreate(BaseLDAPCommand, crud.Create):
     """
     Create a new entry in LDAP.
@@ -1205,6 +1266,8 @@ class LDAPCreate(BaseLDAPCommand, crud.Create):
         except errors.NotFound:
             self.obj.handle_not_found(*keys)
 
+        self.get_indirect_members(entry_attrs, attrs_list)
+
         for callback in self.get_callbacks('post'):
             entry_attrs.dn = callback(
                 self, ldap, entry_attrs.dn, entry_attrs, *keys, **options)
@@ -1328,6 +1391,8 @@ class LDAPRetrieve(LDAPQuery):
         except errors.NotFound:
             self.obj.handle_not_found(*keys)
 
+        self.get_indirect_members(entry_attrs, attrs_list)
+
         if options.get('rights', False) and options.get('all', False):
             entry_attrs['attributelevelrights'] = get_effective_rights(
                 ldap, entry_attrs.dn)
@@ -1476,6 +1541,8 @@ class LDAPUpdate(LDAPQuery, crud.Update):
                 format=_('the entry was deleted while being modified')
             )
 
+        self.get_indirect_members(entry_attrs, attrs_list)
+
         if options.get('rights', False) and options.get('all', False):
             entry_attrs['attributelevelrights'] = get_effective_rights(
                 ldap, entry_attrs.dn)
@@ -1710,6 +1777,8 @@ class LDAPAddMember(LDAPModMember):
         except errors.NotFound:
             self.obj.handle_not_found(*keys)
 
+        self.get_indirect_members(entry_attrs, attrs_list)
+
         for callback in self.get_callbacks('post'):
             (completed, entry_attrs.dn) = callback(
                 self, ldap, completed, failed, entry_attrs.dn, entry_attrs,
@@ -1812,6 +1881,8 @@ class LDAPRemoveMember(LDAPModMember):
         except errors.NotFound:
             self.obj.handle_not_found(*keys)
 
+        self.get_indirect_members(entry_attrs, attrs_list)
+
         for callback in self.get_callbacks('post'):
             (completed, entry_attrs.dn) = callback(
                 self, ldap, completed, failed, entry_attrs.dn, entry_attrs,
@@ -2032,6 +2103,7 @@ class LDAPSearch(BaseLDAPCommand, crud.Search):
 
         if not options.get('raw', False):
             for e in entries:
+                self.get_indirect_members(e, attrs_list)
                 self.obj.convert_attribute_members(e, *args, **options)
 
         for (i, e) in enumerate(entries):
diff --git a/ipalib/plugins/host.py b/ipalib/plugins/host.py
index 41710f3b87b5f60ac6e9b0765a25562b7074e2e8..c47439743da45b8629d1b2afbd210d87591784ce 100644
--- a/ipalib/plugins/host.py
+++ b/ipalib/plugins/host.py
@@ -296,7 +296,7 @@ class host(LDAPObject):
     default_attributes = [
         'fqdn', 'description', 'l', 'nshostlocation', 'krbprincipalname',
         'nshardwareplatform', 'nsosversion', 'usercertificate', 'memberof',
-        'managedby', 'memberindirect', 'memberofindirect', 'macaddress',
+        'managedby', 'memberofindirect', 'macaddress',
         'userclass', 'ipaallowedtoperform', 'ipaassignedidview',
     ]
     uuid_attribute = 'ipauniqueid'
diff --git a/ipalib/plugins/role.py b/ipalib/plugins/role.py
index 55afece22165a550d570fff3aa9f25d103d20b7a..6d8d544aab12023e73bdf3c60db51cef52151aac 100644
--- a/ipalib/plugins/role.py
+++ b/ipalib/plugins/role.py
@@ -71,9 +71,11 @@ class role(LDAPObject):
     object_name_plural = _('roles')
     object_class = ['groupofnames', 'nestedgroup']
     permission_filter_objectclasses = ['groupofnames']
-    default_attributes = ['cn', 'description', 'member', 'memberof',
-        'memberindirect', 'memberofindirect',
-    ]
+    default_attributes = ['cn', 'description', 'member', 'memberof']
+    # Role could have a lot of indirect members, but they are not in
+    # attribute_members therefore they don't have to be in default_attributes
+    # 'memberindirect', 'memberofindirect',
+
     attribute_members = {
         'member': ['user', 'group', 'host', 'hostgroup', 'service'],
         'memberof': ['privilege'],
diff --git a/ipapython/ipaldap.py b/ipapython/ipaldap.py
index ce07006eb790c80fd42bd6eb611732ce9000db13..d4d9309775320a820aed7835e8d8e105a6fc36cb 100644
--- a/ipapython/ipaldap.py
+++ b/ipapython/ipaldap.py
@@ -264,6 +264,8 @@ class IPASimpleLDAPObject(object):
     _SYNTAX_OVERRIDE = CIDict({
         'managedtemplate': DN,
         'managedbase':     DN,
+        'memberindirect':  DN,
+        'memberofindirect':DN,
         'originscope':     DN,
         'idnsname':        DNSName,
         'idnssoamname':    DNSName,
diff --git a/ipaserver/plugins/ldap2.py b/ipaserver/plugins/ldap2.py
index ec491e9e805589fad3bfd5e07ad40ef26581f703..9afdc4cf14609a166e5ce6ec306f98fb05041bde 100644
--- a/ipaserver/plugins/ldap2.py
+++ b/ipaserver/plugins/ldap2.py
@@ -197,102 +197,12 @@ class ldap2(LDAPClient, CrudBackend):
         if size_limit is None:
             size_limit = _get_limits()['size']
 
-        has_memberindirect = False
-        has_memberofindirect = False
-        if attrs_list:
-            new_attrs_list = []
-            for attr_name in attrs_list:
-                if attr_name == 'memberindirect':
-                    has_memberindirect = True
-                elif attr_name == 'memberofindirect':
-                    has_memberofindirect = True
-                else:
-                    new_attrs_list.append(attr_name)
-            attrs_list = new_attrs_list
-
         res, truncated = super(ldap2, self).find_entries(
             filter=filter, attrs_list=attrs_list, base_dn=base_dn, scope=scope,
             time_limit=time_limit, size_limit=size_limit,
             search_refs=search_refs, paged_search=paged_search)
-
-        if has_memberindirect or has_memberofindirect:
-
-            # For the memberof searches, we want to apply the global limit
-            # if it's larger than the requested one, so decreasing limits on
-            # the individual query only affects the query itself.
-            # See https://fedorahosted.org/freeipa/ticket/4398
-            def _max_with_none(a, b):
-                """Maximum of a and b, treating None as infinity"""
-                if a is None or b is None:
-                    return None
-                else:
-                    return max(a, b)
-            time_limit = _max_with_none(time_limit, _get_limits()['time'])
-            size_limit = _max_with_none(size_limit, _get_limits()['size'])
-
-            for entry in res:
-                if has_memberindirect:
-                    self._process_memberindirect(
-                        entry, time_limit=time_limit, size_limit=size_limit)
-                if has_memberofindirect:
-                    self._process_memberofindirect(
-                        entry, time_limit=time_limit, size_limit=size_limit)
-
         return (res, truncated)
 
-    def _process_memberindirect(self, group_entry, time_limit=None,
-                                size_limit=None):
-        filter = self.make_filter({'memberof': group_entry.dn})
-        try:
-            result, truncated = self.find_entries(
-                base_dn=self.api.env.basedn,
-                filter=filter,
-                attrs_list=['member'],
-                time_limit=time_limit,
-                size_limit=size_limit,
-                paged_search=True)
-            if truncated:
-                raise errors.LimitsExceeded()
-        except errors.NotFound:
-            result = []
-
-        indirect = set()
-        for entry in result:
-            indirect.update(entry.get('member', []))
-        indirect.difference_update(group_entry.get('member', []))
-
-        if indirect:
-            group_entry['memberindirect'] = list(indirect)
-
-    def _process_memberofindirect(self, entry, time_limit=None,
-                                  size_limit=None):
-        dn = entry.dn
-        filter = self.make_filter(
-            {'member': dn, 'memberuser': dn, 'memberhost': dn})
-        try:
-            result, truncated = self.find_entries(
-                base_dn=self.api.env.basedn,
-                filter=filter,
-                attrs_list=[''],
-                time_limit=time_limit,
-                size_limit=size_limit)
-            if truncated:
-                raise errors.LimitsExceeded()
-        except errors.NotFound:
-            result = []
-
-        direct = set()
-        indirect = set(entry.get('memberof', []))
-        for group_entry in result:
-            dn = group_entry.dn
-            if dn in indirect:
-                indirect.remove(dn)
-                direct.add(dn)
-
-        entry['memberof'] = list(direct)
-        if indirect:
-            entry['memberofindirect'] = list(indirect)
-
     config_defaults = {'ipasearchtimelimit': [2], 'ipasearchrecordslimit': [0]}
     def get_ipa_config(self, attrs_list=None):
         """Returns the IPA configuration entry (dn, entry_attrs)."""
-- 
2.1.0

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Reply via email to