On 03/25/2014 03:36 PM, Misnyovszki Adam wrote:
On Mon, 24 Mar 2014 17:06:41 +0100
Martin Kosek <mko...@redhat.com> wrote:

On 03/24/2014 11:42 AM, Misnyovszki Adam wrote:
On Fri, 21 Mar 2014 13:06:21 +0100
Petr Viktorin <pvikt...@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 <pvikt...@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 <amisn...@redhat.com> wrote:

On Fri, 14 Mar 2014 13:26:15 -0400
Rob Crittenden <rcrit...@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Ā³

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to