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

[Martin Kosek]

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.

Also, "Task automember" label sounds strange to me, would "Automember task DN"
be better?

Martin