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

[Rob Crittenden]

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.

rob