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

[JR Aquino]

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.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-jraquino-0023-Optimize-and-dynamically-verify-group-membership.patch
Type: application/octet-stream
Size: 6409 bytes
Desc: freeipa-jraquino-0023-Optimize-and-dynamically-verify-group-membership.patch
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20110412/e9916dde/attachment.obj>