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

[Martin Kosek]

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