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

[Martin Kosek]

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.')

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.

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:

Otherwise, this logs INFO log level entry for every migrated user. AFAIU, this
logging density should only be enabled when httpd is run with debug=true.

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)

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.

Martin