On Mon, 24 Mar 2014 17:06:41 +0100
Martin Kosek <mko...@redhat.com> wrote:

> On 03/24/2014 11:42 AM, Misnyovszki Adam wrote:
> > On Fri, 21 Mar 2014 13:06:21 +0100
> > Petr Viktorin <pvikt...@redhat.com> wrote:
> > 
> >> On 03/21/2014 12:58 PM, Martin Kosek wrote:
> >>> On 03/21/2014 12:38 PM, Petr Viktorin wrote:
> >>>> On 03/21/2014 12:00 PM, Misnyovszki Adam wrote:
> >>>>> On Fri, 21 Mar 2014 10:33:00 +0100
> >>>>> Petr Viktorin <pvikt...@redhat.com> wrote:
> >>>>>
> >>>>>> On 03/21/2014 10:29 AM, Petr Viktorin wrote:
> >>>>>>> On 03/20/2014 04:22 PM, Misnyovszki Adam wrote:
> >>>>>>>> On Thu, 20 Mar 2014 14:19:51 +0100
> >>>>>>>> Misnyovszki Adam <amisn...@redhat.com> wrote:
> >>>>>>>>
> >>>>>>>>> On Fri, 14 Mar 2014 13:26:15 -0400
> >>>>>>>>> Rob Crittenden <rcrit...@redhat.com> 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!
> >>>>>>>
> >>>>>>> You forgot an alternate summary for the --no-wait case. (Make
> >>>>>>> sure to output the DN in this case, so it's possible to poll
> >>>>>>> for it.)
> >>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>> Instead of `task['nstaskexitcode'][0]` please use
> >>>>>>> `task.single_value['nstaskexitcode']`.
> >>>>
> >>>> There's one more `task['nstaskexitcode'][0]`. (Perhaps putting it
> >>>> in a variable would be better?)
> >>>>
> >>>>>>>
> >>>>>>> Here:
> >>>>>>>
> >>>>>>>       raise errors.DatabaseError(
> >>>>>>>           desc=_("Automember rebuild membership task failed"),
> >>>>>>>           info=_("nstaskexitcode = '%s'" %
> >>>>>>> str(task['nstaskexitcode'][0])))
> >>>>>>>
> >>>>>>> there's no need to call str() on %'s argument.
> >>>>>>> Also, use natural language (like "Task exit code: %s"),
> >>>>>>> otherwise there's no need to mark the message for translation.
> >>>>>>>
> >>>>>>>
> >>>>>>
> >>>>>> And one more thing: Please bump the minor version in the
> >>>>>> VERSION file when API.txt changes.
> >>>>>>
> >>>>>>
> >>>>>
> >>>>> Hi,
> >>>>> everything is corrected!
> >>>>> Greets
> >>>>> Adam
> >>>>>
> >>>>
> >>>> Sorry for dragging this so long, but:
> >>>> The DN should not be a part of the summary, because that makes it
> >>>> hard to parse when using the API. It should be returned as a part
> >>>> of the result. To do that, you'd need to change the output type:
> >>>>
> >>>>      has_output = output.standard_entry
> >>>>      has_output_params = (
> >>>>          DNParam(
> >>>>              'dn',
> >>>>              label=_('Task automember'),
> >>>>              doc=_('DN of the started task'),
> >>>>          ),
> >>>>      )
> >>>>
> >>>> and provide a dict in result, instead of True. (And with
> >>>> --no-wait, add the dn to that dict.)
> >>>>
> >>>
> >>> Do really want to use 'dn' for the DNParam? dn is already used for
> >>> standard entry DN when --all is used, right? "automembertaskdn"
> >>> may be better.
> >>
> >> Well, I think "DN of the added entry" is exactly what this is.
> >>
> >>> Also, "Task automember" label sounds strange to me, would
> >>> "Automember task DN" be better?
> >>
> >> I meant "Task DN", sorry for the thinko.
> >>
> >>
> > 
> > One more thing, which came to my mind after reviewing the code for
> > myself once again, if the task fails with an exit code other than 0,
> > there is a DatabaseError raised, which is just fine. But in the
> > info, should I specify not only the exit code, but also the DN of
> > the task, or is it unnecessary?
> > Thanks
> > Adam
> 
> DS tasks usually have the error in some of their attributes, so if
> there is such attribute, I would report what the error is. It is
> easier to use than having to run ldapsearch against the given DN.
> 
> If there is not such attribute, providing the DN would be a good idea.
> 
> Martin
Hi,
fixed the output parameter issue, and also with that, API is updated.
If an error occurs, now the function returns the nstaskstatus attribute,
and the DN of the failed task. Also, I added a 3600 second ttl to the
task, because the original was a few seconds(I haven't found the default
value for that anywhere), to ensure that the task doesn't disappear
before interaction is required. Attached the refactor of plugin
registration patch also.

Greets
Adam
>From 1713c1e45e8887a9dc873e70411c1a9e4e16c195 Mon Sep 17 00:00:00 2001
From: Adam Misnyovszki <amisn...@redhat.com>
Date: Tue, 25 Mar 2014 15:15:10 +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 | 41 +++++++++++++++++------------------------
 1 file changed, 17 insertions(+), 24 deletions(-)

diff --git a/ipalib/plugins/automember.py b/ipalib/plugins/automember.py
index 347df39daf761f58c8d51fe94df931c58a736601..103808a8e7f4b28d682b8221dbebb724cfac0971 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, DNParam, _, 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,8 @@ automember_rule = (
     ),
 )
 
+
+@register()
 class automember(LDAPObject):
 
     """
@@ -234,8 +239,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 +247,8 @@ def automember_container_exists(ldap):
         return False
     return True
 
+
+@register()
 class automember_add(LDAPCreate):
     __doc__ = _("""
     Add an automember rule.
@@ -266,9 +271,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 +363,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 +450,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 +465,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 +480,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 +499,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 +514,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 +542,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 +576,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 +600,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:
@@ -754,5 +749,3 @@ class automember_rebuild(Command):
             result=result,
             value=u'',
             summary=summary)
-
-api.register(automember_rebuild)
-- 
1.8.5.3

>From 55e6c3e9ce184d8d6b7dfa5d1166dfc7129d51fe Mon Sep 17 00:00:00 2001
From: Adam Misnyovszki <amisn...@redhat.com>
Date: Tue, 25 Mar 2014 14:47:03 +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,
and returns the DN of the task for further polling.

https://fedorahosted.org/freeipa/ticket/4239
---
 API.txt                      |  7 +++--
 VERSION                      |  4 +--
 ipalib/plugins/automember.py | 70 ++++++++++++++++++++++++++++++++++++--------
 3 files changed, 65 insertions(+), 16 deletions(-)

diff --git a/API.txt b/API.txt
index 326b051e79cb914a2ff2ea603084d7d741f2aa70..633a6958f40f3ab4d3e1dd4248292ecdbc2a64eb 100644
--- a/API.txt
+++ b/API.txt
@@ -201,12 +201,15 @@ 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,7,3
+option: Flag('all', autofill=True, cli_name='all', default=False, exclude='webui')
 option: Str('hosts*')
+option: Flag('no_wait?', autofill=True, default=False)
+option: Flag('raw', autofill=True, cli_name='raw', default=False, exclude='webui')
 option: StrEnum('type', cli_name='type', multivalue=False, required=False, values=(u'group', u'hostgroup'))
 option: Str('users*')
 option: Str('version?', exclude='webui')
-output: Output('result', <type 'bool'>, None)
+output: Entry('result', <type 'dict'>, Gettext('A dictionary representing an LDAP entry', domain='ipa', localedir=None))
 output: Output('summary', (<type 'unicode'>, <type 'NoneType'>), None)
 output: Output('value', <type 'unicode'>, None)
 command: automember_remove_condition
diff --git a/VERSION b/VERSION
index ff5ed2e812e40c2d9c72106db6bb17ba87a3e0e2..afd05da3fcb019ffbccbb485e677c3d391753e8a 100644
--- a/VERSION
+++ b/VERSION
@@ -89,5 +89,5 @@ IPA_DATA_VERSION=20100614120000
 #                                                      #
 ########################################################
 IPA_API_VERSION_MAJOR=2
-IPA_API_VERSION_MINOR=80
-# Last change: pviktori - ":" in permission names
+IPA_API_VERSION_MINOR=81
+# Last change: amisnyov - automember nowait add
diff --git a/ipalib/plugins/automember.py b/ipalib/plugins/automember.py
index a12bfb52522e38bc083d0750dc66c894a4aeba53..347df39daf761f58c8d51fe94df931c58a736601 100644
--- a/ipalib/plugins/automember.py
+++ b/ipalib/plugins/automember.py
@@ -17,8 +17,11 @@
 # 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 import api, errors, Str, StrEnum, DNParam, _, ngettext
 from ipalib.plugins.baseldap import *
 from ipalib.request import context
 from ipapython.dn import DN
@@ -623,9 +626,21 @@ 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_entry
+    has_output_params = (
+        DNParam(
+            'dn',
+            label=_('Task DN'),
+            doc=_('DN of the started task'),
+        ),
     )
-    has_output = output.standard_value
-    msg_summary = _('Automember rebuild membership task completed')
 
     def validate(self, **kw):
         """
@@ -693,20 +708,51 @@ 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'],
+            ttl=[3600])
         ldap.add_entry(entry)
-        return dict(result=True, value=u'')
+
+        if options.get('no_wait'):
+            summary = u"Automember rebuild membership task started"
+            result = {'dn': task_dn}
+        else:
+            summary = u'Automember rebuild membership task completed'
+            result = {}
+            start_time = time.time()
+
+            while True:
+                try:
+                    task = ldap.get_entry(task_dn)
+                except errors.NotFound:
+                    break
+
+                if 'nstaskexitcode' in task:
+                    if str(task.single_value['nstaskexitcode']) == '0':
+                        break
+                    else:
+                        raise errors.DatabaseError(
+                            desc=_(task.single_value['nstaskstatus']),
+                            info=_("Task DN = '%s'" % task_dn))
+                time.sleep(1)
+                if time.time() > (start_time + 60):
+                   raise errors.DatabaseTimeout()
+
+
+        return dict(
+            result=result,
+            value=u'',
+            summary=summary)
 
 api.register(automember_rebuild)
-- 
1.8.5.3

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to