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

[Petr Viktorin]

On 03/14/2014 05:31 PM, 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

Thanks for the patch!

> Request for comments:
> - Should I add a parameter to specify the polling time? (now 1ms)

1ms is way too small, remember this can run on a busy server. I'd make 
it a second, or even several seconds.
I don't think we need an option for it. If someone wants to micromanage 
they're free to use --nowait and poll manually.

> - Should I add a parameter to specify the maximum polling number? Now
>    if something fails about creating the task, it polls forever.

I wouldn't worry about limiting the poll count; if there's a stuck task 
the admin has bigger problems.
Just make sure the polling will stop when the task entry is deleted (!!).

> - Obviously, if these parameters should be added, there should be a
>    reasonable default for them (~ Required=False, Default=X).
>
> Thanks,
> Adam
>
>
> freeipa-amisnyov-0007-1-automember-rebuild-nowait-feature-added.patch
>
>
>>From 62215a10a826d9e529ac861b40c1f1bf68823472 Mon Sep 17 00:00:00 2001
> From: Adam Misnyovszki<amisnyov at redhat.com>
> Date: Fri, 14 Mar 2014 17:22:09 +0100
> Subject: [PATCH] automember rebuild nowait feature added
>
> 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. Old usage can be enabled using --nowait.

Please limit the commit message lines to about 60-75 characters.

>
> https://fedorahosted.org/freeipa/ticket/4239
> ---
>   ipalib/plugins/automember.py | 25 +++++++++++++++++++++++++
>   1 file changed, 25 insertions(+)
>
> diff --git a/ipalib/plugins/automember.py b/ipalib/plugins/automember.py
> index a12bfb52522e38bc083d0750dc66c894a4aeba53..1f36b36b63bf94345f48e18867dbdd3316d6ecb0 100644
> --- a/ipalib/plugins/automember.py
> +++ b/ipalib/plugins/automember.py
> @@ -17,6 +17,7 @@
>   # You should have received a copy of the GNU General Public License
>   # along with this program.  If not, see<http://www.gnu.org/licenses/>.
>   import uuid
> +import time

>   import ldap as _ldap

Style nitpick: When you're touching imports, you can add a blank line 
between standard library imports (uuid, time) and third-party imports 
(ldap), and another one between third-party and IPA imports.

http://legacy.python.org/dev/peps/pep-0008/#imports

>   from ipalib import api, errors, Str, StrEnum, _, ngettext
>   from ipalib.plugins.baseldap import *
> @@ -623,6 +624,13 @@ class automember_rebuild(Command):
>               label=_('Hosts'),
>               doc=_('Rebuild membership for specified hosts'),
>           ),
> +        Flag(
> +            'nowait',
> +            required=False,

We generally specify the 'required' status of an option by a symbol 
following the name:
     'nowait' - required, single value
     'nowait+' - required, multivalue
     'nowait?' - optional, single value
     'nowait*' - optional, multivalue

> +            default=False,
> +            label=_('No wait'),
> +            doc=_('Don\'t wait for rebuilding membership'),

Style nitpick: Use double quotes around the string to avoid the backslash.

> +        ),
>       )
>       has_output = output.standard_value
>       msg_summary = _('Automember rebuild membership task completed')

This may not be true; leave out the msg_summary class attribute and add 
an appropriate 'summary' in execute's return value.

> @@ -707,6 +715,23 @@ class automember_rebuild(Command):
>               scope=['sub']
>           )
>           ldap.add_entry(entry)
> +
> +        while options.get('nowait'):

A subjective style nitpick: Using `while` with a constant is creative, 
but maybe it would be more readable to spell out if+while True.

> +            tasks = ldap.get_entries(
> +                DN(
> +                    ('cn', cn),
> +                    ('cn', 'automember rebuild membership'),
> +                    ('cn', 'tasks'),
> +                    ('cn', 'config'),
> +                ),
> +            )

You can use ldap.get_entry to get a single entry. Both ldap.get_entry 
and ldap.get_entries will raise a NotFound exception if no entries are 
found, you'll need to handle that.

> +            if len(tasks) > 0:

With get_entry that is unnecessary, but in the future, prefer
     if tasks:
for checking if a list is nonempty.

> +                task = tasks[0]
> +                if 'nstaskexitcode' in task.single_value:

An entry's `single_value` dict has the same keys as the entry, so
     if 'nstaskexitcode' in task:
would suffice.

> +                    return dict(result=task.single_value['nstaskexitcode'] == '0', value=unicode(task.single_value['nstaskexitcode']))

If something goes wrong, you should raise an exception, not return 
result=False. Go through ipalib.errors, see if there's a suitable one 
already.

Try to keep lines to 79 characters or less.

> +            time.sleep(0.001)
> +
>           return dict(result=True, value=u'')
>
>   api.register(automember_rebuild)

For extra credit, you could make a separate patch to modernize the 
plugin registration for the whole automember module; see:
http://www.freeipa.org/page/Coding_Best_Practices#Decorator-based_plugin_registration



-- 
Petr³