[Freeipa-devel] [PATCH][RFC] 7 automember rebuild nowait feature added

Misnyovszki Adam amisnyov at redhat.com
Thu Mar 27 14:37:47 UTC 2014 

On Wed, 26 Mar 2014 13:15:55 +0100
Petr Viktorin <pviktori at redhat.com> wrote:

> On 03/25/2014 03:36 PM, Misnyovszki Adam wrote:
> > On Mon, 24 Mar 2014 17:06:41 +0100
> > Martin Kosek <mkosek at redhat.com> wrote:
> >
> >> On 03/24/2014 11:42 AM, Misnyovszki Adam wrote:
> >>> On Fri, 21 Mar 2014 13:06:21 +0100
> >>> Petr Viktorin <pviktori at 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 <pviktori at 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 <amisnyov at redhat.com> wrote:
> >>>>>>>>>>
> >>>>>>>>>>> On Fri, 14 Mar 2014 13:26:15 -0400
> >>>>>>>>>>> Rob Crittenden <rcritten at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-amisnyov-0007-6-automember-rebuild-nowait-feature-added.patch
Type: text/x-patch
Size: 6840 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140327/5194530d/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-amisnyov-0008-plugin-registration-refactoring-for-automembership.patch
Type: text/x-patch
Size: 4966 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140327/5194530d/attachment-0001.bin>

More information about the Freeipa-devel mailing list