On Wed, 26 Mar 2014 13:15:55 +0100
Petr Viktorin <pvikt...@redhat.com> wrote:
> On 03/25/2014 03:36 PM, Misnyovszki Adam wrote:
> > 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
>
>
> Looks great! I'm just concerned about the error returned when the
> task takes too long:
> $ ipa automember-rebuild --type group
> ipa: ERROR: LDAP timeout
> I don't think it's sufficiently clear from this that waiting for the
> task timed out, but the task was actually started successfully. A
> custom error with a more descriptive message would be useful.
>
>
> Also I've noticed that the "nstaskstatus" of a successful task is:
> Automember rebuild task finished. Processed (1) entries.
> This looks helpful; we could return it as the summary.
>
Hi,
both fixed.
Greets
Adam>From e22f55ea61c128b9dd6f54a7cd23367bf5c5ea57 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/errors.py | 16 ++++++++++
ipalib/plugins/automember.py | 71 ++++++++++++++++++++++++++++++++++++--------
4 files changed, 82 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/errors.py b/ipalib/errors.py
index 716decb2b41baf5470a1dc23c0cfb5d1c995e5ff..3fc6fffe6c1abf0c9dfcdf72e2e714a17a91be29 100644
--- a/ipalib/errors.py
+++ b/ipalib/errors.py
@@ -1512,6 +1512,22 @@ class DatabaseTimeout(DatabaseError):
format = _('LDAP timeout')
+class TaskTimeout(DatabaseError):
+ """
+ **4212** Raised when an LDAP task times out
+
+ For example:
+
+ >>> raise TaskTimeout()
+ Traceback (most recent call last):
+ ...
+ TaskTimeout: Automember LDAP task timeout, Task DN: ''
+ """
+
+ errno = 4212
+ format = _("%(task)s LDAP task timeout, Task DN: '%(task_dn)s'")
+
+
class CertificateError(ExecutionError):
"""
**4300** Base class for Certificate execution errors (*4300 - 4399*).
diff --git a/ipalib/plugins/automember.py b/ipalib/plugins/automember.py
index a12bfb52522e38bc083d0750dc66c894a4aeba53..81ce8418032be8b10d80a6fbbf1b4b08ae03f676 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,52 @@ 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':
+ summary=task.single_value['nstaskstatus']
+ 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.TaskTimeout(task='Automember', task_dn=task_dn)
+
+
+ return dict(
+ result=result,
+ value=u'',
+ summary=summary)
api.register(automember_rebuild)
--
1.9.0
>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
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
