Issue #47451: add/enable/disable/remove plugins without server restart - 389-ds-base

In addition, we should allow one to change the pluginConfigArea attribute for plug-ins that support it without requiring a restart.

nkinder commented 10 years ago

Linked to Bugzilla bug: https://bugzilla.redhat.com/show_bug.cgi?id=994690 (''Red Hat Enterprise Linux 6'')

mreynolds commented 10 years ago

Replying to [comment:2 nkinder]:

In addition, we should allow one to change the pluginConfigArea attribute for plug-ins that support it without requiring a restart.

This was just completed for the RI and memberOf plugins via tickets 47525 & 47603.

mreynolds commented 10 years ago

Replying to [comment:7 nkinder]:

I missed this milestone change, but I'm very close to finishing this.

rmeggins commented 10 years ago

I'm going to have to review a chunk at a time, so please bear with me.

It looks like every plugin has to maintain its own reference counter. Can this be centralized in the plugin code e.g. add a counter to struct slapdplugin? The plugin.c code knows exactly when a plugin function is called and returns, and knows when the plugin close function is called.

mreynolds commented 10 years ago

Replying to [comment:10 rmeggins]:

I'm going to have to review a chunk at a time, so please bear with me.

Understood, I know its a big patch, and thanks for starting to tackle it. FYI, most of the tricky work is all in plugin.c.

It looks like every plugin has to maintain its own reference counter. Can this be centralized in the plugin code e.g. add a counter to struct slapdplugin?

I will definitely look into this. The op counter code was the very last thing I did, and I wasn't exactly thrilled about it, but we need to have something like it in place.

rmeggins commented 10 years ago

The rest of the code looks good.

mreynolds commented 10 years ago

Replying to [comment:12 rmeggins]:

The rest of the code looks good.

The new patch is attached. I added the counter to the plugin structure, so the counter init and destroy is handled when a plugin is started and stopped. There a few plugins where I could not use this: statechange, posix winsync, etc, as these use special registered callback functions that do have have access the plug-in structure. So those plugins are using there own private counters. Also, tasks do not have access to the plugin structure, so I'm using a task ref counter for those plugin task functions.

rmeggins commented 10 years ago

Why do the acct_policy, pam_passthru, passthru, retrocl, roles, rootdn plugins have to call slapi_plugin_op_?
Do the automember, dna, linked_attrs, member_of, mep, referint, usn, views plugins have to call slapi_plugin_op_ because they may be running as tasks? The calls are not in the functions exclusively used for tasks.

posix-winsync is a winsync plugin, not a "regular" plugin. It (and statechange, and a couple of other plugins) use the slapi api broker plugin code. Will we need to do some additional work on those types of plugins?

mreynolds commented 10 years ago

Replying to [comment:14 rmeggins]:

Why do the acct_policy, pam_passthru, passthru, retrocl, roles, rootdn plugins have to call slapi_plugin_op_*?

That's how the op counter is incremented/decremented.

I could move some of this code out of the individual plugin code, and into plugin_call_func() (start and finish plugin op calls), but we still need slapi_plugin_op_all_finished() in the close function - so we know when it's safe to free the resources.

Do the automember, dna, linked_attrs, member_of, mep, referint, usn, views plugins have to call slapi_plugin_op_* because they may be running as tasks?

For tasks, I also had to add a separate counter, local to the plugin. The current op counter implementation is storing the counter in the plugin entry(inside of the pblock). Tasks can not access this plugin entry and they often create their own threads, so I had use a separate global ref counter for these tasks. So there are technically two counters used, one for active operations, and one for tasks.

The same for some callbacks, they don't have access to the pblock/plugin either.

The calls are not in the functions exclusively used for tasks.

posix-winsync is a winsync plugin, not a "regular" plugin. It (and statechange, and a couple of other plugins) use the slapi api broker plugin code. Will we need to do some additional work on those types of plugins?

Well, I used a single local/separate counter for these plugins. I could try and modify the api broker code, but we would need two counters then - instead of one central one for the entire plugin.

So, at the end of day I could not use the exact same method of operation counting for all the plugins and tasks. I'm not sure we can implement a 100%, behind the scenes, global plugin operation counter(not without making some major changes). I can look into it more closely, but it would mean redoing the task code, api-broker, callbacks?, etc. Thoughts? I'm open to ideas.

rmeggins commented 10 years ago

Replying to [comment:15 mreynolds]:

Replying to [comment:14 rmeggins]:

Why do the acct_policy, pam_passthru, passthru, retrocl, roles, rootdn plugins have to call slapi_plugin_op_*?

That's how the op counter is incremented/decremented.

I could move some of this code out of the individual plugin code, and into plugin_call_func() (start and finish plugin op calls),

That's what I would prefer. It is already quite difficult to write plugins correctly - this is just one more thing that will trip people up.

but we still need slapi_plugin_op_all_finished() in the close function - so we know when it's safe to free the resources.

Can't that be done in plugin_call_plugins() or wherever the plugin close function is called?

Do the automember, dna, linked_attrs, member_of, mep, referint, usn, views plugins have to call slapi_plugin_op_* because they may be running as tasks?

For tasks, I also had to add a separate counter, local to the plugin. The current op counter implementation is storing the counter in the plugin entry(inside of the pblock). Tasks can not access this plugin entry and they often create their own threads, so I had use a separate global ref counter for these tasks. So there are technically two counters used, one for active operations, and one for tasks.

The same for some callbacks, they don't have access to the pblock/plugin either.

The calls are not in the functions exclusively used for tasks.

posix-winsync is a winsync plugin, not a "regular" plugin. It (and statechange, and a couple of other plugins) use the slapi api broker plugin code. Will we need to do some additional work on those types of plugins?

Well, I used a single local/separate counter for these plugins. I could try and modify the api broker code, but we would need two counters then - instead of one central one for the entire plugin.

So, at the end of day I could not use the exact same method of operation counting for all the plugins and tasks. I'm not sure we can implement a 100%, behind the scenes, global plugin operation counter(not without making some major changes). I can look into it more closely, but it would mean redoing the task code, api-broker, callbacks?, etc. Thoughts? I'm open to ideas.

I'm not sure. But I do know that we need to make it easier to write plugins, not harder. So the more we can do behind the scenes, automatically, the better.

mreynolds commented 10 years ago

Replying to [comment:16 rmeggins]:

I could move some of this code out of the individual plugin code, and into plugin_call_func() (start and finish plugin op calls),

That's what I would prefer. It is already quite difficult to write plugins correctly - this is just one more thing that will trip people up.

but we still need slapi_plugin_op_all_finished() in the close function - so we know when it's safe to free the resources.

Can't that be done in plugin_call_plugins() or wherever the plugin close function is called?

Yeah I was just thinking about that too.

So, at the end of day I could not use the exact same method of operation counting for all the plugins and tasks. I'm not sure we can implement a 100%, behind the scenes, global plugin operation counter(not without making some major changes). I can look into it more closely, but it would mean redoing the task code, api-broker, callbacks?, etc. Thoughts? I'm open to ideas.

I'm not sure. But I do know that we need to make it easier to write plugins, not harder.

Agreed.

So the more we can do behind the scenes, automatically, the better.

Ok, I'll continue to work on this.

tbordaz commented 10 years ago

This is an impressive amount of work. I have one question regarding 'plugin_delete'. This function is called when you disable/remove a plugin.

If this plugin init function registered others plugins (slapi_register_plugin), then 'plugin_delete' removes those plugins from the plugin list based on the plugin->plg_dn. That is fine.

Now my understand is that you may pick up the wrong plugin list. In fact there is a plugin list per plugin type. There is nothing that guarantee that 'group' plugin type has the same type than the 'member' plugin type. Actually it is even common than a 'group' plugin registers 'member' plugins from different types. For example, 'pam' plugin that is 'betxnpreoperation', registers three plugins (preop, internalpreop and betxnpostoperation).

It is looking like that the type (so the plugin list) of 'member' is only defined in the plugin init function. Shouldn't be the group plugin terminaison function that should unregister the 'member' plugins ?

mreynolds commented 10 years ago

Replying to [comment:18 tbordaz]:

This is an impressive amount of work. I have one question regarding 'plugin_delete'. This function is called when you disable/remove a plugin.

If this plugin init function registered others plugins (slapi_register_plugin), then 'plugin_delete' removes those plugins from the plugin list based on the plugin->plg_dn. That is fine.

Now my understand is that you may pick up the wrong plugin list. In fact there is a plugin list per plugin type. There is nothing that guarantee that 'group' plugin type has the same type than the 'member' plugin type. Actually it is even common than a 'group' plugin registers 'member' plugins from different types. For example, 'pam' plugin that is 'betxnpreoperation', registers three plugins (preop, internalpreop and betxnpostoperation).

It is looking like that the type (so the plugin list) of 'member' is only defined in the plugin init function. Shouldn't be the group plugin terminaison function that should unregister the 'member' plugins ?

Actually plugin_remove_plugins() scans all the plugin type lists for member functions. This was something I ran into very early on in my work. So yes, for example a preop plugins can register, internalpreop, extendop, etc, functions, and this will correctly identify those plugins and remove them.

rmeggins commented 10 years ago

If I understand this correctly, any change to any plugin parameter, from plugin path to include/exclude subtree attributes to log access/audit, will cause the plugin to be deleted then re-added back. This seems a bit heavyweight. For some changes e.g. plugin path, initfn, type, etc. there really is no other way. But for example, if I just wanted to add a subtree to the exclusion list, or enable audit logging for a plugin, I probably don't want to delete and add the plugin, which will wipe out all of the data in memory stored by the plugin. On the other hand, maybe this is good enough for now, and we can separate the handling of the parameters which cause a plugin restart from the others in a separate ticket.

The dse callbacks are problematic. We are taking away the dse callback argument. I don't think we can do that. I think we need a new function e.g. slapi_config_register_callback_plugin which must be used for any plugin that wants to be dynamically deleted/added. This function is exactly the same as slapi_config_register_callback except that it has a *pb argument which is used to associated the dse callback with the slapi plugin. Then, I think DSE_FLAG_PLUGIN won't be needed.

mreynolds commented 10 years ago

Replying to [comment:22 rmeggins]:

If I understand this correctly, any change to any plugin parameter, from plugin path to include/exclude subtree attributes to log access/audit, will cause the plugin to be deleted then re-added back. This seems a bit heavyweight. For some changes e.g. plugin path, initfn, type, etc. there really is no other way. But for example, if I just wanted to add a subtree to the exclusion list, or enable audit logging for a plugin, I probably don't want to delete and add the plugin, which will wipe out all of the data in memory stored by the plugin. On the other hand, maybe this is good enough for now, and we can separate the handling of the parameters which cause a plugin restart from the others in a separate ticket.

We could definitely separate it into two types of config attributes, but I would like to do it in a separate ticket as this will require a non-trivial change, and this patch is large enough as it is.

The dse callbacks are problematic. We are taking away the dse callback argument. I don't think we can do that.

Well the callback argument wasn't really being used, so I thought I could take advantage of that. The reason I did that was to reduce the impact on existing plugins, and for writers of new plugins. I already added some new API functions, was just trying to keep it to a minimum. Anyway, no problem, I can add the new function.

I think we need a new function e.g. slapi_config_register_callback_plugin which must be used for any > plugin that wants to be dynamically deleted/added. This function is exactly the same as >slapi_config_register_callback except that it has a *pb argument which is used to associated the dse >callback with the slapi plugin. Then, I think DSE_FLAG_PLUGIN won't be needed.

True, we can move it all behind the scenes with a new function.

rmeggins commented 10 years ago

Replying to [comment:23 mreynolds]:

Replying to [comment:22 rmeggins]:

If I understand this correctly, any change to any plugin parameter, from plugin path to include/exclude subtree attributes to log access/audit, will cause the plugin to be deleted then re-added back. This seems a bit heavyweight. For some changes e.g. plugin path, initfn, type, etc. there really is no other way. But for example, if I just wanted to add a subtree to the exclusion list, or enable audit logging for a plugin, I probably don't want to delete and add the plugin, which will wipe out all of the data in memory stored by the plugin. On the other hand, maybe this is good enough for now, and we can separate the handling of the parameters which cause a plugin restart from the others in a separate ticket.

Yes, agreed.

We could definitely separate it into two types of config attributes, but I would like to do it in a separate ticket as this will require a non-trivial change, and this patch is large enough as it is.

The dse callbacks are problematic. We are taking away the dse callback argument. I don't think we can do that.

Well the callback argument wasn't really being used, so I thought I could take advantage of that.

They are being used, and in addition, since they are part of the public API, someone could be using them, and we would break that.

The reason I did that was to reduce the impact on existing plugins, and for writers of new plugins.

There is already going to be an impact on plugins that use DSE callbacks - there is no way around that - unless there is some tricky way to associate a DSE callback with a plugin, but I don't see how that is possible.

I already added some new API functions, was just trying to keep it to a minimum. Anyway, no problem, I can add the new function.

I think we need a new function e.g. slapi_config_register_callback_plugin which must be used for any > plugin that wants to be dynamically deleted/added. This function is exactly the same as >slapi_config_register_callback except that it has a *pb argument which is used to associated the dse >callback with the slapi plugin. Then, I think DSE_FLAG_PLUGIN won't be needed.

True, we can move it all behind the scenes with a new function.

mreynolds commented 10 years ago

New patch attached. The patch is still in the coverity queue, but there are 12 jobs ahead of it. Not sure when its actually going to complete, so while I'm waiting I figured it would be best to get this patch out for review in the meantime.

rmeggins commented 10 years ago

Looking good. One minor nit:
{{{
3385 PR_snprintf(returntext, SLAPI_DSE_RETURNTEXT_SIZE,"Plugin delete failed: dependency error "
3386 "prevents the plugin from being deleted/disabled.");
}}}
text should say which plugin it is. same with
{{{
3161 LDAPDebug(LDAP_DEBUG_ANY, "plugin_delete_check_dependency: this plugin type is needed "
3162 "by other plugins, it can not be dynamically changed at this time. This could "
3163 "cause issues when the server is restarted.\n",0,0,0);
}}}
and
{{{
LDAPDebug(LDAP_DEBUG_ANY, "plugin_delete_check_dependency: this plugin type is needed "
3166 "by other plugins, it can not be disabled/moved at this time.\n",0,0,0);
}}}

mreynolds commented 10 years ago

revision - Improved error logging
0001-Ticket-47451-add-enable-disable-remove-plugins-witho.patch

mreynolds commented 10 years ago

Replying to [comment:26 rmeggins]:

Looking good. One minor nit:
{{{
3385 PR_snprintf(returntext, SLAPI_DSE_RETURNTEXT_SIZE,"Plugin delete failed: dependency error "
3386 "prevents the plugin from being deleted/disabled.");
}}}
text should say which plugin it is. same with
{{{
3161 LDAPDebug(LDAP_DEBUG_ANY, "plugin_delete_check_dependency: this plugin type is needed "
3162 "by other plugins, it can not be dynamically changed at this time. This could "
3163 "cause issues when the server is restarted.\n",0,0,0);
}}}
and
{{{
LDAPDebug(LDAP_DEBUG_ANY, "plugin_delete_check_dependency: this plugin type is needed "
3166 "by other plugins, it can not be disabled/moved at this time.\n",0,0,0);
}}}

Okay the error message (error log and client returntext) will list all the plugins that rely on this plugin type when the dependency check fails New patch is attached...

rmeggins commented 10 years ago

ack!

mreynolds commented 10 years ago

Thanks Rich!

git merge dynamic-plugin
Updating e4a1de6..dbfab50
Fast-forward
ldap/ldif/template-dse.ldif.in | 1 +
ldap/schema/01core389.ldif | 1 +
.../plugins/acct_usability/acct_usability.c | 9 +-
ldap/servers/plugins/acctpolicy/acct_config.c | 9 +
ldap/servers/plugins/acctpolicy/acct_init.c | 19 +
ldap/servers/plugins/acctpolicy/acctpolicy.h | 1 +
ldap/servers/plugins/acl/acl.h | 1 +
ldap/servers/plugins/acl/acl_ext.c | 16 +-
ldap/servers/plugins/acl/aclgroup.c | 4 +-
ldap/servers/plugins/acl/aclplugin.c | 1 +
ldap/servers/plugins/automember/automember.c | 135 +--
ldap/servers/plugins/chainingdb/cb.h | 7 +-
ldap/servers/plugins/chainingdb/cb_close.c | 76 +-
ldap/servers/plugins/chainingdb/cb_instance.c | 60 +-
ldap/servers/plugins/cos/cos.c | 3 +-
ldap/servers/plugins/cos/cos_cache.c | 14 +-
ldap/servers/plugins/deref/deref.c | 9 +-
ldap/servers/plugins/dna/dna.c | 70 +-
ldap/servers/plugins/linkedattrs/fixup_task.c | 25 +-
ldap/servers/plugins/linkedattrs/linked_attrs.c | 70 +-
ldap/servers/plugins/linkedattrs/linked_attrs.h | 3 +
ldap/servers/plugins/memberof/memberof.c | 40 +-
ldap/servers/plugins/memberof/memberof.h | 4 +-
ldap/servers/plugins/memberof/memberof_config.c | 47 +-
ldap/servers/plugins/mep/mep.c | 63 +-
ldap/servers/plugins/pam_passthru/pam_passthru.h | 3 +-
ldap/servers/plugins/pam_passthru/pam_ptconfig.c | 9 +
ldap/servers/plugins/pam_passthru/pam_ptimpl.c | 9 +
ldap/servers/plugins/pam_passthru/pam_ptpreop.c | 41 +-
ldap/servers/plugins/passthru/passthru.h | 5 +
ldap/servers/plugins/passthru/ptconfig.c | 483 ++++----
ldap/servers/plugins/passthru/ptpreop.c | 15 +-
.../plugins/posix-winsync/posix-group-func.c | 7 +
.../plugins/posix-winsync/posix-group-func.h | 1 +
.../plugins/posix-winsync/posix-winsync-config.c | 23 +
ldap/servers/plugins/posix-winsync/posix-winsync.c | 128 ++-
.../plugins/posix-winsync/posix-wsp-ident.h | 5 +
ldap/servers/plugins/referint/referint.c | 23 +-
ldap/servers/plugins/retrocl/retrocl.c | 32 +-
ldap/servers/plugins/retrocl/retrocl_po.c | 13 +-
ldap/servers/plugins/roles/roles_cache.c | 12 +-
ldap/servers/plugins/roles/roles_plugin.c | 20 +-
ldap/servers/plugins/rootdn_access/rootdn_access.c | 27 +-
ldap/servers/plugins/schema_reload/schema_reload.c | 35 +-
ldap/servers/plugins/statechange/statechange.c | 48 +-
ldap/servers/plugins/sync/sync.h | 1 +
ldap/servers/plugins/sync/sync_init.c | 5 +-
ldap/servers/plugins/sync/sync_persist.c | 43 +-
ldap/servers/plugins/sync/sync_refresh.c | 11 +
ldap/servers/plugins/usn/usn.c | 27 +-
ldap/servers/plugins/usn/usn.h | 1 -
ldap/servers/plugins/usn/usn_cleanup.c | 13 +-
ldap/servers/plugins/views/views.c | 111 ++-
ldap/servers/plugins/whoami/whoami.c | 5 +-
ldap/servers/slapd/apibroker.c | 2 +
ldap/servers/slapd/config.c | 11 +-
ldap/servers/slapd/configdse.c | 2 +-
ldap/servers/slapd/dse.c | 287 ++++-
ldap/servers/slapd/factory.c | 210 ++--
ldap/servers/slapd/fedse.c | 48 +-
ldap/servers/slapd/libglobs.c | 31 +
ldap/servers/slapd/main.c | 8 +-
ldap/servers/slapd/plugin.c | 1426 +++++++++++++++++---
ldap/servers/slapd/proto-slap.h | 14 +-
ldap/servers/slapd/result.c | 1 -
ldap/servers/slapd/schema.c | 10 +-
ldap/servers/slapd/slap.h | 11 +
ldap/servers/slapd/slapi-plugin.h | 55 +
ldap/servers/slapd/task.c | 52 +-
ldap/servers/slapd/thread_data.c | 56 +-
ldap/servers/slapd/tools/pwenc.c | 7 +-
71 files changed, 2914 insertions(+), 1161 deletions(-)

git push origin master
e4a1de6..dbfab50 master -> master

commit dbfab50
Author: Mark Reynolds mreynolds@redhat.com
Date: Thu Mar 20 16:09:43 2014 -0400

389-ds-base

Source Code

#47451 add/enable/disable/remove plugins without server restart Closed: wontfix None Opened 10 years ago by rmeggins.

Metadata

#47451 add/enable/disable/remove plugins without server restart

Closed: wontfix None Opened 10 years ago by rmeggins.