[Freeipa-devel] [PATCH] 1083 improve migration performance

[Martin Kosek]

On 02/05/2013 03:27 PM, Rob Crittenden wrote:
> Martin Kosek wrote:
>> On 02/04/2013 08:05 PM, Rob Crittenden wrote:
>>> Martin Kosek wrote:
>>>> On 02/01/2013 04:21 PM, Rob Crittenden wrote:
>>>>> I did some analysis on migration and found several areas impacting
>>>>> performance:
>>>>>
>>>>> 1. We were calling user_mod to reset the magic value in description to not
>>>>> create a UPG. This caused a lot of unnecessary queries to display the user.
>>>>>
>>>>> 2. We check the remote LDAP server to make sure that the GID is valid and
>>>>> added
>>>>> a cache. We lacked a negative cache.
>>>>>
>>>>> 3. The biggest drag on performance was managing the default users group.
>>>>> After
>>>>> about 1000 users it would take about half a second to calculate the
>>>>> modlist and
>>>>> another half second for 389-ds to apply the change.
>>>>>
>>>>> This patch addresses all of these.
>>>>>
>>>>> For the last what I do is only do the group addition every 100 records. A
>>>>> query
>>>>> is run to find all users who aren't in the default users group and those are
>>>>> added.
>>>>>
>>>>> I also added a bit of logging so one can better track the progress of
>>>>> migration.
>>>>>
>>>>> I migrated 12.5k users with compat enabled in 3 1/2 hours.
>>>>>
>>>>> I migrated the same 12.5k users and 2k groups with compat disabled in 30
>>>>> minutes.
>>>>>
>>>>> By contrast when I started, with compat enabled, I migrated:
>>>>>
>>>>> 1000 users in 7 minutes
>>>>> 2000 users in 27 minutes
>>>>> 3000 users in 1 hour
>>>>>
>>>>> rob
>>>>>
>>>>
>>>> Good job, this should improve the migration plugin perfomance a lot. Just few
>>>> minor remarks:
>>>>
>>>> 1) I am not native speaker, but this looks strange to me:
>>>>
>>>> +_krb_failed_msg = _('Unable to determine Kerberos principal %s already
>>>> exists.
>>>> Use \'ipa user-mod\' to set it manually.')
>>>
>>> Yup, typo on my part.
>>>
>>>>
>>>> Shouldn't it read "Unable to determine IF Kerberos principal..."?
>>>>
>>>> 2) In:
>>>>
>>>> +        searchfilter = "(&(objectclass=posixAccount)(!(memberof=%s)))" %
>>>> group_dn
>>>> +        (result, truncated) = ldap.find_entries(searchfilter,
>>>> +            ['member'], api.env.container_user, scope=_ldap.SCOPE_SUBTREE,
>>>> +            time_limit = -1)
>>>>
>>>> Shouldn't we search with empty attrs_list ("attrs_list=['']")? We do not need
>>>> nor use the member attribute anyway.
>>>
>>> Sure. I had it in my head that asking for an empty list returned everything. I
>>> did some quick testing and asking for a blank attribute returns no attributes,
>>> so I think will work.
>>>
>>>>
>>>> 3) In
>>>>
>>>> +                if migrate_cnt > 0 and migrate_cnt % 100:
>>>> +                    api.log.info("%d %ss migrated. %s elapsed." %
>>>> (migrate_cnt, ldap_obj_name, total_dur))
>>>>
>>>> I think you wanted to do this condition:
>>>>
>>>> if migrate_cnt > 0 and migrate_cnt % 100 == 0:
>>>
>>> Yup, this is what I get for adding something right before submitting the patch.
>>> Fixed.
>>>
>>>
>>>> 4) In _update_default_group:
>>>>
>>>> +        api.log.debug('Adding users to group duration %s' % d)
>>>>
>>>> I would improve it this way:
>>>>
>>>> +        mode = " (forced)" if force else ""
>>>> +        api.log.debug('Adding %d users to group%s, duration %s', migrate_cnt,
>>>> mode, d)
>>>
>>> Sure, added.
>>>
>>>>
>>>> 5) We now print a lot of interesting migration-related  information to IPA
>>>> server httpd error_log. I think it may be useful to also add a note about
>>>> it to
>>>> "ipa help migration" I think that regular admins may not have a clue that we
>>>> log information like this to this error log.
>>>
>>> I added a short logging section.
>>>
>>> rob
>>>
>>
>> Almost there!
>>
>> 1) Few new typos:
>> +recommended that this be evaluated post-migration to correct or
>>                         ^^
> 
> I think this was correct but it was definitely awkward so I re-worded the
> sentence.
> 
>> ...
>> +/etc/ipa/defult.conf or /etc/ipa/server.conf, then an entry will be printed
>>            ^^^^^^^^^^^
> 
> Fixed
> 
>> 2) ipausers group update may report error for the whole migration if there is
>> no user to update (e.g. if all migrated users already exist in IPA):
>>
>> # ipa migrate-ds --with-compat...
>> ipa: ERROR: no such entry
>>
>> This is the misbehaving LDAP search:
>> +        searchfilter = "(&(objectclass=posixAccount)(!(memberof=%s)))" %
>> group_dn
>> +        (result, truncated) = ldap.find_entries(searchfilter,
>> +            [''], api.env.container_user, scope=_ldap.SCOPE_SUBTREE,
>> +            time_limit = -1)
> 
> Ok, wrapped in try/except.
> 
> I also moved the ldap.update_entry() block so we don't try the update if we
> know we have no new members.
> 
> rob

ACK. Pushed to master, ipa-3-1, ipa-3-0.

Martin