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

Wed Mar 26 12:15:55 UTC 2014

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.

-- 
Petr³