Hi,
I have tried to adapt the memberOf plugin as it exists in today in the CVS to our production environment - it is a very nice and USEFUL addition to server features, especially if one has to manage direct and indirect groups.
We have several relatively large groups (~ 3000 entries). It turned out that at each full group members delete and recreation I've had huge memory leaks (~ 100 Mb per each full delete/recreation). I've tried to narrow it down in the plug-in. here is what i have found (i structured it in paragraphs) :
§1.There are several slapi_search_internal_get_entry() calls that return a copy of Slapi_Entry *e that is never released by slapi_entry_free(). Here are the proposed changes : --- /tmp/fedora-ds-base-cvs/ldap/servers/plugins/memberof/memberof.c 2008-02-19 07:04:56.000000000 +0100 +++ /tmp/memberof-patched.c 2008-03-23 22:13:00.000000000 +0100 @@ -1019,6 +1019,7 @@ }
bail: + slapi_entry_free(e); return rc; }
@@ -1310,6 +1311,7 @@ }
slapi_sdn_free(&sdn); + slapi_entry_free(group_e); return rc; }
@@ -1826,6 +1828,8 @@
bail: slapi_ch_free_string(&filter_str); + slapi_entry_free(group_e); + slapi_entry_free(opto_e);
slapi_log_error( SLAPI_LOG_TRACE, MEMBEROF_PLUGIN_SUBSYSTEM, "<-- memberof_is_legit_member\n" );
§2. I don't know whether the arrays pre_array and post_array in memberofd_replace_list() function should be freed. I think they should because they are allocated by slapi_ch_malloc(), but i'm not sure and i don't know how i should free them - i am not a specialist of the plugin API. Anyway, apparently this part of code is never used by our group management applications so i don't have a memory leak caused by this function.
§3. If we make MODRDN on an INDIRECT group the original indirect group DN is deleted from memberof but the renamed indirect group DN is not added. It may be fixed by running every once in a while the memberOf fixup task BUT in this particular case the attribute will never be fixed - because of the problem in §4 :
§4. I propose to delete all the memberOf attribute values at the beginning of the fix thread. Otherwise if we have in the entry a "memberOf" attribute value equal to a DN of a direct group, the memeberOf fix task (the function memberof_fix_memberof_callback()) will NEVER add the indirect groupsDNs that depend on the abovementioned direct group, so this thread is not really a FULL FIX thread :) Here are the proposed changes to avoid it: --- /tmp/fedora-ds-base-cvs/ldap/servers/plugins/memberof/memberof.c 2008-02-19 07:04:56.000000000 +0100 +++ /tmp/memberof-patched.c 2008-03-23 22:13:00.000000000 +0100 @@ -2006,6 +2010,7 @@ /* memberof_fix_memberof_callback() * Add initial and/or fix up broken group list in entry * + * 0. Delete all the memberOf attribute values * 1. Make sure direct membership groups are in the entry * 2. Add all groups that current group list allows through nested membership * 3. Trim groups that have no relationship to entry @@ -2016,6 +2021,9 @@ char *dn = slapi_entry_get_dn(e); memberof_add_groups data = {dn, dn};
+ /* step 0. */ + slapi_entry_attr_delete(e, MEMBEROF_ATTR); + /* step 1. and step 2. */ rc = memberof_call_foreach_dn(0, dn, MEMBEROF_GROUP_ATTR, memberof_add_groups_search_callback, &data);
§5. After the code changes proposed in "§1" i still have ~600 bytes memory increase per 5000 modifications (2500 DEL & 2500 ADDs to group membership, the memory increases mainly at DEL operations). I don't know whether it is due to connection management in slapd code or to the plug-in itself but it is a much more acceptable value for me than 100Mb :)
§6. #define MEMBEROF_GROUP_ATTR_TYPE "uid" is declared but never used in the plug-in code...
Thanks once more for this plug-in!
Andrey Ivanov wrote:
Hi,
I have tried to adapt the memberOf plugin as it exists in today in the CVS to our production environment - it is a very nice and USEFUL addition to server features, especially if one has to manage direct and indirect groups.
We have several relatively large groups (~ 3000 entries). It turned out that at each full group members delete and recreation I've had huge memory leaks (~ 100 Mb per each full delete/recreation). I've tried to narrow it down in the plug-in. here is what i have found (i structured it in paragraphs) :
§1.There are several slapi_search_internal_get_entry() calls that return a copy of Slapi_Entry *e that is never released by slapi_entry_free(). Here are the proposed changes : --- /tmp/fedora-ds-base-cvs/ldap/servers/plugins/memberof/memberof.c 2008-02-19 07:04:56.000000000 +0100 +++ /tmp/memberof-patched.c 2008-03-23 22:13:00.000000000 +0100 @@ -1019,6 +1019,7 @@ }
bail:
slapi_entry_free(e); return rc;
}
@@ -1310,6 +1311,7 @@ }
slapi_sdn_free(&sdn);
slapi_entry_free(group_e); return rc;
}
@@ -1826,6 +1828,8 @@
bail: slapi_ch_free_string(&filter_str);
slapi_entry_free(group_e);
slapi_entry_free(opto_e); slapi_log_error( SLAPI_LOG_TRACE, MEMBEROF_PLUGIN_SUBSYSTEM, "<-- memberof_is_legit_member\n" );
§2. I don't know whether the arrays pre_array and post_array in memberofd_replace_list() function should be freed. I think they should because they are allocated by slapi_ch_malloc(), but i'm not sure and i don't know how i should free them - i am not a specialist of the plugin API. Anyway, apparently this part of code is never used by our group management applications so i don't have a memory leak caused by this function.
§3. If we make MODRDN on an INDIRECT group the original indirect group DN is deleted from memberof but the renamed indirect group DN is not added. It may be fixed by running every once in a while the memberOf fixup task BUT in this particular case the attribute will never be fixed - because of the problem in §4 :
§4. I propose to delete all the memberOf attribute values at the beginning of the fix thread. Otherwise if we have in the entry a "memberOf" attribute value equal to a DN of a direct group, the memeberOf fix task (the function memberof_fix_memberof_callback()) will NEVER add the indirect groupsDNs that depend on the abovementioned direct group, so this thread is not really a FULL FIX thread :) Here are the proposed changes to avoid it: --- /tmp/fedora-ds-base-cvs/ldap/servers/plugins/memberof/memberof.c 2008-02-19 07:04:56.000000000 +0100 +++ /tmp/memberof-patched.c 2008-03-23 22:13:00.000000000 +0100 @@ -2006,6 +2010,7 @@ /* memberof_fix_memberof_callback()
- Add initial and/or fix up broken group list in entry
- Delete all the memberOf attribute values
- Make sure direct membership groups are in the entry
- Add all groups that current group list allows through nested membership
- Trim groups that have no relationship to entry
@@ -2016,6 +2021,9 @@ char *dn = slapi_entry_get_dn(e); memberof_add_groups data = {dn, dn};
/* step 0. */
slapi_entry_attr_delete(e, MEMBEROF_ATTR);
/* step 1. and step 2. */ rc = memberof_call_foreach_dn(0, dn, MEMBEROF_GROUP_ATTR, memberof_add_groups_search_callback, &data);
§5. After the code changes proposed in "§1" i still have ~600 bytes memory increase per 5000 modifications (2500 DEL & 2500 ADDs to group membership, the memory increases mainly at DEL operations). I don't know whether it is due to connection management in slapd code or to the plug-in itself but it is a much more acceptable value for me than 100Mb :)
§6. #define MEMBEROF_GROUP_ATTR_TYPE "uid" is declared but never used in the plug-in code...
Thanks once more for this plug-in!
Thank you for the information Andrey. I'm currently doing some cleanup on the memberOf plug-in around the implementation of the cleanup task. Once I am finished with that work, I'll look into the issues that you raised.
We are working on making the memberOf plug-in more robust, as it's currently in a sort of "alpha" stage as you've noticed. If you are interested in helping with this effort (or just following it), take a look at the design document for this plug-in on our wiki (http://directory.fedoraproject.org/wiki/MemberOf_Plugin).
Thanks again! -NGK
389-devel@lists.fedoraproject.org