[Freeipa-devel] [PATCH] 23 Optimize and dynamically verify group membership

[Rob Crittenden]

JR Aquino wrote:
> On Apr 12, 2011, at 10:55 AM, Rob Crittenden wrote:
>
>> JR Aquino wrote:
>>> On Apr 7, 2011, at 7:08 PM, JR Aquino wrote:
>>>
>>>>
>>>> On Apr 7, 2011, at 4:04 PM, JR Aquino wrote:
>>>>
>>>>> On Apr 7, 2011, at 3:42 PM, JR Aquino wrote:
>>>>>
>>>>>> On Mar 31, 2011, at 2:16 PM, JR Aquino wrote:
>>>>>>
>>>>>>> On Mar 31, 2011, at 1:48 PM, Rob Crittenden wrote:
>>>>>>>
>>>>>>>> JR Aquino wrote:
>>>>>>>>> The following patch Removes around 20 lines of code and provides a substantial increase in performance for FreeIPA member/memberof verification searches.
>>>>>>>>>
>>>>>>>>> The current code base blindly searches static containers for the possible presence of members.
>>>>>>>>>
>>>>>>>>> This patch provides a method for dynamically identifying the specific objects to verify memberships for.
>>>>>>>>>
>>>>>>>>> The attached patch addresses ticket:
>>>>>>>>> https://fedorahosted.org/freeipa/ticket/1139
>>>>>>>>>
>>>>>>>>> <Without patch>
>>>>>>>>>
>>>>>>>>> ipa hostgroup-find
>>>>>>>>>
>>>>>>>>> ...
>>>>>>>>>
>>>>>>>>> -----------------------------
>>>>>>>>> Number of entries returned 52
>>>>>>>>> -----------------------------
>>>>>>>>>
>>>>>>>>> real	0m20.054s
>>>>>>>>> user	0m0.934s
>>>>>>>>> sys	0m0.050s
>>>>>>>>>
>>>>>>>>> <With Patch>
>>>>>>>>> ipa find-hostgroup
>>>>>>>>>
>>>>>>>>> ...
>>>>>>>>>
>>>>>>>>> -----------------------------
>>>>>>>>> Number of entries returned 52
>>>>>>>>> -----------------------------
>>>>>>>>>
>>>>>>>>> real	0m15.064s
>>>>>>>>> user	0m0.945s
>>>>>>>>> sys	0m0.057s
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> ------------------------------
>>>>>>>>> Number of entries returned 100
>>>>>>>>> ------------------------------
>>>>>>>>>
>>>>>>>>> real	0m16.471s
>>>>>>>>> user	0m0.814s
>>>>>>>>> sys	0m0.040s
>>>>>>>>>
>>>>>>>>> <Without Patch>
>>>>>>>>> ipa host-find
>>>>>>>>>
>>>>>>>>> ...
>>>>>>>>>
>>>>>>>>> ------------------------------
>>>>>>>>> Number of entries returned 100
>>>>>>>>> ------------------------------
>>>>>>>>>
>>>>>>>>> real	0m41.277s
>>>>>>>>> user	0m0.806s
>>>>>>>>> sys	0m0.060s
>>>>>>>>>
>>>>>>>>> <With Patch>
>>>>>>>>> ipa host-find
>>>>>>>>>
>>>>>>>>> ...
>>>>>>>>>
>>>>>>>>> ------------------------------
>>>>>>>>> Number of entries returned 100
>>>>>>>>> ------------------------------
>>>>>>>>>
>>>>>>>>> real	0m16.385s
>>>>>>>>> user	0m0.814s
>>>>>>>>> sys	0m0.053s
>>>>>>>>
>>>>>>>> There is a typo in the first block, memeber.
>>>>>>>>
>>>>>>>> Wouldn't it be clearer to do a negative test to continue:
>>>>>>>>
>>>>>>>> if not 'member' in r[1]:
>>>>>>>> continue
>>>>>>>>
>>>>>>>> rob
>>>>>>>
>>>>>>> You're right!
>>>>>>>
>>>>>>> Corrected patch attached.
>>>>>>
>>>>>> Self Nack
>>>>>>
>>>>>> After cli and webui testing, it turned out there was a previous try / except block that was reseting the results value back to []
>>>>>>
>>>>>> Corrected and reattaching new patch.
>>>>>>
>>>>>> Testing cli and webui checks out correctly. Speed AND accuracy are now addressed.
>>>>>>
>>>>>> It was also discovered during the course of testing that this patch addresses one of the causes for the bug thrown in: https://fedorahosted.org/freeipa/ticket/1133
>>>>>>
>>>>>> -JR
>>>>>
>>>>> NACK
>>>>>
>>>>> Looks like there may still need to be work with the indirect / direct functions.
>>>>>
>>>>> Will revisit next week.
>>>>
>>>> Ok I finally think I've got it.
>>>>
>>>> My for loop was in my try / except block. It has now been corrected.
>>>>
>>>> I've tested the searches for: users, groups, sudocmds, sudcmdgroups, sudorules, hosts, hostgroups, hbacrules, hbacsv, hbsvcgroups, and all return as expected.
>>>>
>>>> Please make sure that they return for you as well.
>>>> Please let me know if there is anything else I have missed.
>>>
>>> Final Patch attached due to relationship with ticket 1133:
>>>
>>> Added Comments regarding Ticket 1133 / calculating indirect:
>>>
>>> +        # If there is an exception here, it is due to a failure in referential integrity.
>>> +        # All members should have corresponding memberOf entries.
>>>
>>> Retested against all xmlrpc tests and passed.
>>
>> Seems to work as advertised, I just have a couple of requests:
>>
>> - Some of the comments are really long, can you limit to ~75 chars per line?
>> - In this code block:
>>
>>         for r in results:
>>             direct.append(r[0])
>>             try:
>>                 indirect.remove(r[0].lower())
>>             except ValueError:
>>                 pass
>>
>> We should log if a ValueError is thrown, this would mean something is really wrong. Can you import logging in the file and replace pass with something like:
>>
>> logging.info('Failed to remove indirect entry %s from %s' % r[0], entry_dn)
>>
>> I wonder if we should raise the ValueError too. This means that something went very wrong.
>
> Yes I agree that we should raise the error.
>
> Here is the patch with the requested changes:
>
> * Fixed width to be PEP8 compliant
> * import logging
> * Added logging line in exception
> * Raise ValueError if we blow up during indirect removal.
>

Fixes 1-3 look good. I think for the exception you want:

except ValueError, e:
    logging ....
    raise e

A ValueError won't get returned over XML-RPC but the full backtrace will 
be logged on the server side. The end-user will get a 903 (Internal 
error raised).

rob