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<[email protected]>
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³
_______________________________________________
Freeipa-devel mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/freeipa-devel
