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

[Petr Viktorin]

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.


-- 
Petr³