[SSSD] [PATCH] Use dereference to speed up RFC2307bis group processing

Jakub Hrozek jhrozek at redhat.com
Fri May 20 11:15:35 UTC 2011


On 05/20/2011 12:10 PM, Stephen Gallagher wrote:
> On Wed, 2011-05-18 at 16:41 +0200, Jakub Hrozek wrote:
>> On 05/09/2011 07:23 PM, Stephen Gallagher wrote:
>>> Continuing my review. Comments inline.
>>>
>>
>> New patches are attached. They are formatted with -M -C --patience
>> --full-index, per Simo's suggestion. I think it helped, especially with
>> patch #1.
>>
>> I'd like to merge the replies into one thread. Below are replies for the
>> first half of patches.
>>
>> Patch #1 has no changes.
>>
>> Patch #2 is a new one that just defines a couple of data structures and
>> one function that are used in later patches in order to process MPGs.
>>
>> Patches #3 and #4 (previously #2 and #3) that implement the
>> platform-specific dereference methods are now able to dereference MPGs,
>> too. The OpenLDAP patch also removes one unused variable.
>>
>> If an LDAP object that matches multiple maps is dereferenced, it is
>> returned as multiple sysdb_attrs * results plus the corresponding map to
>> speed up processing.
>>
>> I tested the functionality by dereferencing an object that was both a
>> posixUser and a posixGroup. The dereference itself worked fine and both
>> objects were saved to sysdb but it seems that the rest of the LDAP
>> provider assumes that originalDN is unique -- so later the back end
>> failed when it was trying to build memberships. This is something to fix
>> if we want to support MPGs, but I don't think it is related to this
>> patchset.
>>
>> Patch #5 (previously #4) now has single sdap_deref_search_done()
>> function instead of one per method.
>>
>> The patch that changed append_attrs_to_array was retired. I will send a
>> separate patch to remove that function altogether as it is not used
>> anywhere.
>>
>> Patch #6 has no changes.
>>
>>>
>>> On Wed, 2011-05-04 at 13:19 +0200, Jakub Hrozek wrote:
>>>>
>>>> [PATCH 7/9] Use fake users during RFC2307bis nested group processing
>>>> Instead of downloading complete user data which is potentionally very
>>>> slow, only download the necessary minimum information and store the
>>>> users as dummy entries.
>>>>
>>>
>>> Nack.
>>> sdap_nested_group_populate_users() doesn't properly sanitize the origDN
>>> before using it in a search filter.
>>>
>>
>> Fixed.
>>
>>>> [PATCH 8/9] Refactor RFC2307bis nested group processing
>>>> This patch splits checking cache and hash tables into standalone
>>>> functions. This will make it easy to reuse the code in a new branch that
>>>> uses dereferencing.
>>>>
>>>
>>> Nack.
>>>
>>> I realize this was a problem in my original code, but would you mind
>>> fixing two issues with sdap_nested_group_check_cache() while you're
>>> there?
>>>
>>> 1) Please rearrange the code so that all of the user processing takes
>>> place before the group processing. It's not terribly readable the way
>>> the complete group processing happens in the middle. It would be better
>>> to have the return value from sysdb_search_users() set mtype either to
>>> SYSDB_MEMBER_USER or SYSDB_MEMBER_GROUP (assumed) and then have another
>>> IF statement after that so we can do all of the user processing in
>>> series.
>>>
>>> 2) It's kind of wasteful to allocate filter twice in the case of groups.
>>> Better would be to hang onto it until we're sure we're not processing a
>>> group. Then we don't need to allocate and free the exact same filter
>>> twice.
>>>
>>> Please change the comment in the ENOENT block of
>>> sdap_nested_group_process_step() since it no longer makes sense with the
>>> user and group split in a sub-function.
>>>
>>>
>>
>> Fixed
>>
>>>
>>>> [PATCH 9/9] Use dereference in processing RFC2307bis nested groups
>>>> Instead of issuing N LDAP requests when processing a group with N users,
>>>> utilize the dereference functionality to pull down all the members in a
>>>> single LDAP request.
>>>>
>>>> https://fedorahosted.org/sssd/ticket/799
>>>
>>> Nack.
>>>
>>> In sdap_nested_group_process_deref_step(), if
>>> sdap_nested_group_check_cache() returns something other than EOK, ENOENT
>>> or EAGAIN, it should be returned as an error. Right now, you're treating
>>> all errors as ENOENT.
>>>
>>
>> Fixed
>>
>>> Regarding the tuning for when to perform the deref, I don't have a
>>> perfect answer either. My only suggestion would be to make the value
>>> configurable in sssd.conf. I think we should get rid of the percentage
>>> and make it purely based on number of members. The way to tune it should
>>> be that we should deref whenever a deref will take less time to process
>>> than N members. I think we should default to ten members.
>>>
>>
>> Making it configurable is a good answer :-) and I agree that the option
>> should only specify absolute values.
> 
> 
> 
> Patch 0001: Ack
> Patch 0002: Ack
> Patch 0003: Ack
> Patch 0004: Ack
> Patch 0005: Ack
> Patch 0006: Ack
> Patch 0007: Ack
> Patch 0008: Ack
> Patch 0009: Nack. Please add ldap_deref_threshold to sssd-ipa.conf (not
> just sssd-ldap.conf.
> 

Done. I also noticed that ldap_page size was missing, so I added that
one, too.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-sdap_get_generic_ext.patch
Type: text/x-patch
Size: 15516 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20110520/de25bb18/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Generic-dereference-data-structures-and-utilities.patch
Type: text/x-patch
Size: 2604 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20110520/de25bb18/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-Add-support-for-Attribute-Scoped-Queries.patch
Type: text/x-patch
Size: 7884 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20110520/de25bb18/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-OpenLDAP-dereference-searches.patch
Type: text/x-patch
Size: 13978 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20110520/de25bb18/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0005-Generic-dereference-search.patch
Type: text/x-patch
Size: 6540 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20110520/de25bb18/attachment-0004.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0006-Change-sysdb_add_fake_user-to-add-OriginalDN.patch
Type: text/x-patch
Size: 2543 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20110520/de25bb18/attachment-0005.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0007-Use-fake-users-during-RFC2307bis-nested-group-proces.patch
Type: text/x-patch
Size: 9411 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20110520/de25bb18/attachment-0006.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0008-Refactor-RFC2307bis-nested-group-processing.patch
Type: text/x-patch
Size: 14322 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20110520/de25bb18/attachment-0007.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0009-Use-dereference-when-processing-RFC2307bis-nested-gr.patch
Type: text/x-patch
Size: 24894 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20110520/de25bb18/attachment-0008.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 261 bytes
Desc: OpenPGP digital signature
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20110520/de25bb18/attachment.sig>


More information about the sssd-devel mailing list