On Thu, 20 Mar 2014 14:19:51 +0100 Misnyovszki Adam <[email protected]> wrote:
> On Fri, 14 Mar 2014 13:26:15 -0400 > Rob Crittenden <[email protected]> wrote: > > > Misnyovszki Adam wrote: > > > Hi, > > > > > > automember-rebuild uses asynchronous 389 task, and returned > > > success even if the task didn't run. This patch fixes this issue > > > adding a --nowait parameter to 'ipa automember-rebuild', > > > defaulting to False, thus when the script runs without it, it > > > waits for the 'nstaskexitcode' attribute, which means the task > > > has finished, according to > > > http://directory.fedoraproject.org/wiki/Task_Invocation_Via_LDAP#Implementation. > > > Old usage can be enabled using --nowait. > > > > > > https://fedorahosted.org/freeipa/ticket/4239 > > > > > > Request for comments: > > > - Should I add a parameter to specify the polling time? (now 1ms) > > > - Should I add a parameter to specify the maximum polling number? > > > Now if something fails about creating the task, it polls forever. > > > - Obviously, if these parameters should be added, there should be > > > a reasonable default for them (~ Required=False, Default=X). > > > > I don't think you need a polling time, esp since this is hidden from > > the user, but I think that is probably too short and you may end up > > hammering the LDAP server. > > > > I also wonder if there should be some maximum wait time. I don't > > like loops that can never exit. I'm at a loss for what that time > > should be though. And we'd need to spell out that we gave up > > waiting, not that the task necessarily failed. So rather than > > having a polling time option, rename nowait into wait_for=20, so > > wait for 20 seconds. Or something like that. > > > > I'd suggest using get_entry since you already know the full DN and > > there is only ever one. It would make this much more readable. > > > > I wonder if some top-level documentation should be added to flesh > > this out some more. This does, for example, return False in one > > case. The meaning for that should be spelled out. > > > > rob > > Hi, > personally I would keep --no-wait, with a delay of 1 seconds, and a > maximum waiting time for 60 seconds. Questions is, do we need to > parameterize with other parameters(wait-for to the maximum time, > and/or poll-delay for the delay time, both not required), or it is > not a case which worth to develop? > Greets > Adam Hi, here are the corrections Petr asked, also the other patch conatins the plugin registration refactor. Thanks Adam > > _______________________________________________ > Freeipa-devel mailing list > [email protected] > https://www.redhat.com/mailman/listinfo/freeipa-devel
>From 32c4fdb505b02a582afbff65366382216982359d Mon Sep 17 00:00:00 2001 From: Adam Misnyovszki <[email protected]> Date: Thu, 20 Mar 2014 15:50:29 +0100 Subject: [PATCH] automember rebuild nowait feature added automember-rebuild uses asynchronous 389 task, and returned success even if the task didn't run. this patch fixes this issue adding a --nowait parameter to 'ipa automember-rebuild', defaulting to False, thus when the script runs without it, it waits for the 'nstaskexitcode' attribute, which means the task has finished. Old usage can be enabled using --nowait. https://fedorahosted.org/freeipa/ticket/4239 --- API.txt | 3 ++- ipalib/plugins/automember.py | 52 +++++++++++++++++++++++++++++++++++--------- 2 files changed, 44 insertions(+), 11 deletions(-) diff --git a/API.txt b/API.txt index 8e1f7713ade2b3dc046e9db82fdd6be2d85eec56..52959922241d0df11556c2890bf56d7b8107ed62 100644 --- a/API.txt +++ b/API.txt @@ -201,10 +201,11 @@ output: Entry('result', <type 'dict'>, Gettext('A dictionary representing an LDA output: Output('summary', (<type 'unicode'>, <type 'NoneType'>), None) output: Output('value', <type 'unicode'>, None) command: automember_rebuild -args: 0,4,3 +args: 0,5,3 option: Str('hosts*') option: StrEnum('type', cli_name='type', multivalue=False, required=False, values=(u'group', u'hostgroup')) option: Str('users*') +option: Flag('no_wait?', autofill=True, default=False) option: Str('version?', exclude='webui') output: Output('result', <type 'bool'>, None) output: Output('summary', (<type 'unicode'>, <type 'NoneType'>), None) diff --git a/ipalib/plugins/automember.py b/ipalib/plugins/automember.py index a12bfb52522e38bc083d0750dc66c894a4aeba53..feb2cd7637fcdbf31bf5dea86be1667ffd51876b 100644 --- a/ipalib/plugins/automember.py +++ b/ipalib/plugins/automember.py @@ -17,7 +17,10 @@ # You should have received a copy of the GNU General Public License # along with this program. If not, see <http://www.gnu.org/licenses/>. import uuid +import time + import ldap as _ldap + from ipalib import api, errors, Str, StrEnum, _, ngettext from ipalib.plugins.baseldap import * from ipalib.request import context @@ -623,9 +626,14 @@ class automember_rebuild(Command): label=_('Hosts'), doc=_('Rebuild membership for specified hosts'), ), + Flag( + 'no_wait?', + default=False, + label=_('No wait'), + doc=_("Don't wait for rebuilding membership"), + ), ) has_output = output.standard_value - msg_summary = _('Automember rebuild membership task completed') def validate(self, **kw): """ @@ -693,20 +701,44 @@ class automember_rebuild(Command): else: search_filter = '(%s=*)' % obj.primary_key.name + task_dn = DN( + ('cn', cn), + ('cn', 'automember rebuild membership'), + ('cn', 'tasks'), + ('cn', 'config')) + entry = ldap.make_entry( - DN( - ('cn', cn), - ('cn', 'automember rebuild membership'), - ('cn', 'tasks'), - ('cn', 'config'), - ), + task_dn, objectclass=['top', 'extensibleObject'], cn=[cn], basedn=[basedn], filter=[search_filter], - scope=['sub'] - ) + scope=['sub']) ldap.add_entry(entry) - return dict(result=True, value=u'') + + no_wait = options.get('no_wait') + if not no_wait: + start_time = time.time() + while True: + try: + task = ldap.get_entry(task_dn) + except errors.NotFound: + break + + if 'nstaskexitcode' in task: + if str(task['nstaskexitcode'][0]) == '0': + break + else: + raise errors.DatabaseError( + desc=_("Automember rebuild membership task failed"), + info=_("nstaskexitcode = '%s'" % str(task['nstaskexitcode'][0]))) + time.sleep(1) + if time.time() > (start_time + 60): + raise errors.DatabaseTimeout() + + return dict( + result=True, + value=u'', + summary=u'Automember rebuild membership task completed') api.register(automember_rebuild) -- 1.8.5.3
>From e6da821cdd9196a427a3db7fb35a3d0252f32171 Mon Sep 17 00:00:00 2001 From: Adam Misnyovszki <[email protected]> Date: Thu, 20 Mar 2014 16:10:50 +0100 Subject: [PATCH] plugin registration refactoring for automembership decorators used for plugin registration in automembership according to: http://www.freeipa.org/page/Coding_Best_Practices#Decorator-based_plugin_registration --- ipalib/plugins/automember.py | 40 ++++++++++++++++------------------------ 1 file changed, 16 insertions(+), 24 deletions(-) diff --git a/ipalib/plugins/automember.py b/ipalib/plugins/automember.py index feb2cd7637fcdbf31bf5dea86be1667ffd51876b..bb14d3f3f54ca7041622db24ba1316a471db4b0f 100644 --- a/ipalib/plugins/automember.py +++ b/ipalib/plugins/automember.py @@ -22,6 +22,7 @@ import time import ldap as _ldap from ipalib import api, errors, Str, StrEnum, _, ngettext +from ipalib.plugable import Registry from ipalib.plugins.baseldap import * from ipalib.request import context from ipapython.dn import DN @@ -124,6 +125,8 @@ EXAMPLES: ipa automember-rebuild --hosts=web1.example.com --hosts=web2.example.com """) +register = Registry() + # Options used by Condition Add and Remove. INCLUDE_RE = 'automemberinclusiveregex' EXCLUDE_RE = 'automemberexclusiveregex' @@ -167,6 +170,7 @@ automember_rule = ( ), ) +@register() class automember(LDAPObject): """ @@ -234,8 +238,6 @@ class automember(LDAPObject): else: raise errors.NotFound(reason=_('%s is not a valid attribute.') % attr) -api.register(automember) - def automember_container_exists(ldap): try: @@ -244,6 +246,8 @@ def automember_container_exists(ldap): return False return True + +@register() class automember_add(LDAPCreate): __doc__ = _(""" Add an automember rule. @@ -266,9 +270,8 @@ class automember_add(LDAPCreate): result['value'] = keys[-1] return result -api.register(automember_add) - +@register() class automember_add_condition(LDAPUpdate): __doc__ = _(""" Add conditions to an automember rule. @@ -359,9 +362,8 @@ class automember_add_condition(LDAPUpdate): result['value'] = keys[-1] return result -api.register(automember_add_condition) - +@register() class automember_remove_condition(LDAPUpdate): __doc__ = _(""" Remove conditions from an automember rule. @@ -447,9 +449,8 @@ class automember_remove_condition(LDAPUpdate): result['value'] = keys[-1] return result -api.register(automember_remove_condition) - +@register() class automember_mod(LDAPUpdate): __doc__ = _(""" Modify an automember rule. @@ -463,9 +464,8 @@ class automember_mod(LDAPUpdate): result['value'] = keys[-1] return result -api.register(automember_mod) - +@register() class automember_del(LDAPDelete): __doc__ = _(""" Delete an automember rule. @@ -479,9 +479,8 @@ class automember_del(LDAPDelete): result['value'] = keys[-1] return result -api.register(automember_del) - +@register() class automember_find(LDAPSearch): __doc__ = _(""" Search for automember rules. @@ -499,9 +498,8 @@ class automember_find(LDAPSearch): ndn = DN(('cn', options['type']), base_dn) return (filters, ndn, scope) -api.register(automember_find) - +@register() class automember_show(LDAPRetrieve): __doc__ = _(""" Display information about an automember rule. @@ -515,9 +513,8 @@ class automember_show(LDAPRetrieve): result['value'] = keys[-1] return result -api.register(automember_show) - +@register() class automember_default_group_set(LDAPUpdate): __doc__ = _(""" Set default (fallback) group for all unmatched entries. @@ -544,9 +541,8 @@ class automember_default_group_set(LDAPUpdate): result['value'] = options['type'] return result -api.register(automember_default_group_set) - +@register() class automember_default_group_remove(LDAPUpdate): __doc__ = _(""" Remove default (fallback) group for all unmatched entries. @@ -579,9 +575,8 @@ class automember_default_group_remove(LDAPUpdate): result['value'] = options['type'] return result -api.register(automember_default_group_remove) - +@register() class automember_default_group_show(LDAPRetrieve): __doc__ = _(""" Display information about the default (fallback) automember groups. @@ -604,9 +599,8 @@ class automember_default_group_show(LDAPRetrieve): result['value'] = options['type'] return result -api.register(automember_default_group_show) - +@register() class automember_rebuild(Command): __doc__ = _('Rebuild auto membership.') # TODO: Add a --dry-run option: @@ -740,5 +734,3 @@ class automember_rebuild(Command): result=True, value=u'', summary=u'Automember rebuild membership task completed') - -api.register(automember_rebuild) -- 1.8.5.3
_______________________________________________ Freeipa-devel mailing list [email protected] https://www.redhat.com/mailman/listinfo/freeipa-devel
