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

[Rob Crittenden]

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-rcrit-1083-3-migrate.patch
Type: text/x-diff
Size: 10218 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130205/de963dd1/attachment.bin>