<html>
<head>
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix">On 02/26/2016 05:48 PM, Martin Babinsky
wrote:<br>
</div>
<blockquote cite="mid:56D081E4.4070702@redhat.com" type="cite">On
02/26/2016 04:24 PM, thierry bordaz wrote:
<br>
<blockquote type="cite">On 02/25/2016 07:17 PM, thierry bordaz
wrote:
<br>
<blockquote type="cite">On 02/25/2016 12:03 PM, Martin Babinsky
wrote:
<br>
<blockquote type="cite">On 02/24/2016 04:30 PM, thierry bordaz
wrote:
<br>
<blockquote type="cite">On 01/21/2016 05:04 PM, Martin
Babinsky wrote:
<br>
<blockquote type="cite">On 01/21/2016 01:37 PM, thierry
bordaz wrote:
<br>
<blockquote type="cite">
<br>
<br>
<br>
</blockquote>
Hi Thierry,
<br>
<br>
I have couple of comments to your patch:
<br>
<br>
1.)
<br>
there is a number of PEP8 errors in the patch
<br>
(<a class="moz-txt-link-freetext" href="http://paste.fedoraproject.org/313246/33893701">http://paste.fedoraproject.org/313246/33893701</a>), please
fix them.
<br>
<br>
See <a class="moz-txt-link-freetext" href="http://www.freeipa.org/page/Python_Coding_Style">http://www.freeipa.org/page/Python_Coding_Style</a> for
our
<br>
conventions used in Python code.
<br>
<br>
2.)
<br>
+ DNA_BIND_METHOD = "dnaRemoteBindMethod"
<br>
+ DNA_CONN_PROTOCOL = "dnaRemoteConnProtocol"
<br>
+ DNA_PLUGIN_DN = 'cn=Distributed Numeric
Assignment
<br>
Plugin,cn=plugins,cn=config'
<br>
+ dna_config_base = 'cn=Posix IDs,%s' %
DNA_PLUGIN_DN
<br>
<br>
Uppercase names are usually reserved for module-level
constants. OTOH,
<br>
local variables should be lowercase. Also you can
instantiate
<br>
dna_config_base directly as DN, using 2-member tuples,
i. e:
<br>
<br>
"""
<br>
dna_config_base = DN(('cn', 'posix IDs'), ('cn',
'Distributed Numeric
<br>
Assignment Plugin'), ('cn', 'plugins'), ('cn',
'config'))
<br>
"""
<br>
<br>
When passing DN object to the formatting
functions/operators, it is
<br>
automatically converted to string so no need to hold
string and DN
<br>
object separately. This is done in other places (see
function/methods
<br>
in replication.py).
<br>
<br>
3.)
<br>
<br>
+ for i in range(len(entries)) :
<br>
+
<br>
+ mod = []
<br>
+ if
entries[i].single_value.get(DNA_BIND_METHOD) !=
<br>
method:
<br>
+ mod.append((ldap.MOD_REPLACE,
DNA_BIND_METHOD,
<br>
method))
<br>
+
<br>
+ if
entries[i].single_value.get(DNA_CONN_PROTOCOL) !=
<br>
protocol:
<br>
+ mod.append((ldap.MOD_REPLACE,
DNA_CONN_PROTOCOL,
<br>
protocol))
<br>
<br>
<br>
please use idiomatic Python when handling list of
entries, e.g.:
<br>
<br>
"""
<br>
for entry in entries:
<br>
mod = []
<br>
if entry.single_value.get(DNA_BIND_METHOD) != method:
<br>
...
<br>
"""
<br>
<br>
4.) I think that this method should in DSInstance class
since it deals
<br>
with directory server configuration. Service is a parent
object of all
<br>
other service installers/configurators and should
contain only methods
<br>
common to more children.
<br>
<br>
5.) Since the method is called from every installer, it
could be
<br>
beneficial to call it in
DSInstance.__common_post_setup() as a part of
<br>
Directory server installation. Is there any reason why
this is not the
<br>
case?
<br>
<br>
6.)
<br>
<br>
+ while attempt != MAX_WAIT:
<br>
+ try:
<br>
+ entries = conn.get_entries(sharedcfgdn,
<br>
scope=ldap.SCOPE_ONELEVEL, filter='dnaHostname=%s' %
self.fqdn)
<br>
+ break
<br>
+ except errors.NotFound:
<br>
+ root_logger.debug("So far enable not
find DNA shared
<br>
config entry for dnaHostname=%s under %s. Retry in 2sec"
% (self.fqdn,
<br>
sharedcfgdn))
<br>
+ attempt = attempt + 1
<br>
+ time.sleep(2)
<br>
+ continue
<br>
+
<br>
+ # safety checking
<br>
+ # there is no return, if there are several
entries, as a
<br>
workaround of #5510
<br>
+ if len(entries) != 1:
<br>
<br>
I am quite afraid what would happen if the server does
not return any
<br>
entries until 30 s timeout. The code will then continue
to the
<br>
condition which can potentially test an uninitialized
variable and
<br>
blow up with 'NameError'. This should be handled more
robustly, e. g.
<br>
raise an exception when a timeout is reached and no
entries were
<br>
returned.
<br>
<br>
7.)
<br>
<br>
+ if len(mod) > 0:
<br>
<br>
A Pythonic way to test for non-empty container is
<br>
<br>
"""
<br>
if mods:
<br>
# do stuff
<br>
"""
<br>
<br>
since an empty list/dict/set evaluates to False and
non-empty
<br>
containers are True.
<br>
<br>
<br>
8.)
<br>
<br>
+ entry = conn.get_entry(entries[i].dn)
<br>
+ if
entry.single_value.get(DNA_BIND_METHOD) != method:
<br>
+ root_logger.error("Fail to set
SASL/GSSAPI bind
<br>
method to %s" % (entries[i].dn))
<br>
+ if
entry.single_value.get(DNA_CONN_PROTOCOL) !=
<br>
protocol:
<br>
+ root_logger.error("Fail to set LDAP
protocol to
<br>
%s" % (entries[i].dn))
<br>
<br>
rather than re-fetching the modified entry and testing
what happened,
<br>
you can just catch an exception raised by unsuccessfull
mod and log an
<br>
error like this:
<br>
<br>
"""
<br>
try:
<br>
conn.modify_s(entry.dn, mod)
<br>
except Exception as e:
<br>
root_logger.error("Failed to modify entry {}:
{}".format(entry, e))
<br>
"""
<br>
<br>
as a matter of fact, if the modify_s operation would
fail for some
<br>
reason, an ldap exception would be raised and you checks
would not
<br>
even be executed.
<br>
<br>
9.)
<br>
The debug message on line 219 should read "Unable to
find DNA shared
<br>
config entry for dnaHostname=%s so far. Retry in 2
sec.". The errors
<br>
at the end of the method should have "Failed" instead of
"Fail".
<br>
<br>
</blockquote>
Hi Martin,
<br>
<br>
Finally tested... here is the updated patch. Thanks for
you patience
<br>
<br>
<br>
thanks
<br>
thierry
<br>
</blockquote>
<br>
Hi Thierry,
<br>
<br>
the patch works as expected. I have some more nitpicks
though:
<br>
<br>
1.) Please fix the following pep8 errors:
<br>
<br>
<a class="moz-txt-link-freetext" href="http://paste.fedoraproject.org/329086/56397841/">http://paste.fedoraproject.org/329086/56397841/</a>
<br>
<br>
you can check whether you recent commit conforms to PEP8 by
running
<br>
<br>
"git show -U0 | pep8 --diff"
<br>
<br>
2.)
<br>
+ self.step("update DNA shared config entry",
<br>
self.update_dna_shared_config)
<br>
<br>
I would rather change the message to "Updating DNA shared
config
<br>
entry" since all other messages use continuous tense.
<br>
<br>
3.)
<br>
+ else:
<br>
+ raise RuntimeError("Could not get
dnaHostname
<br>
entries in {} seconds".format(max_wait * 2))
<br>
<br>
Please use root_logger.error() and then return as is used
elsewhere
<br>
in the method. We do not want a runaway exception crashing
upgrade.
<br>
<br>
</blockquote>
Hi Martin,
<br>
<br>
Updated/tested the patch with your help/recommendations. pep8
is clear
<br>
now :-)
<br>
<br>
thanks
<br>
thierry
<br>
</blockquote>
<br>
Hi Martin,
<br>
<br>
Following your recommendation it is an updated patch to not
check/update
<br>
shared config entry in DSinstance.__post_common_setup().
<br>
In fact at this step, DNA plugin is disabled and such the check
would be
<br>
a no-op.
<br>
<br>
thanks
<br>
thierry
<br>
</blockquote>
<br>
Thanks Thierry,
<br>
<br>
the patch will need a rebased version which applies cleanly on top
of ipa-4-3 branch, but otherwise ACK.
<br>
<br>
</blockquote>
<font face="Times New Roman, Times, serif">Thanks Martin for all the
reviews.<br>
<br>
Here is the patch for ipa-4.3<br>
<br>
thnaks<br>
thierry<br>
</font>
</body>
</html>