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

[Martin Kosek]

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
                       ^^
...
+/etc/ipa/defult.conf or /etc/ipa/server.conf, then an entry will be printed
          ^^^^^^^^^^^

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)

Martin