On 02/25/2016 01:56 PM, Martin Babinsky wrote:
On 02/25/2016 12:17 PM, thierry bordaz wrote:
On 02/25/2016 12:03 PM, Martin Babinsky wrote:
On 02/24/2016 04:30 PM, thierry bordaz wrote:
On 01/21/2016 05:04 PM, Martin Babinsky wrote:
On 01/21/2016 01:37 PM, thierry bordaz wrote:




Hi Thierry,

I have couple of comments to your patch:

1.)
there is a number of PEP8 errors in the patch
(http://paste.fedoraproject.org/313246/33893701), please fix them.

See http://www.freeipa.org/page/Python_Coding_Style for our
conventions used in Python code.

2.)
+        DNA_BIND_METHOD   = "dnaRemoteBindMethod"
+        DNA_CONN_PROTOCOL = "dnaRemoteConnProtocol"
+        DNA_PLUGIN_DN     = 'cn=Distributed Numeric Assignment
Plugin,cn=plugins,cn=config'
+        dna_config_base   = 'cn=Posix IDs,%s' % DNA_PLUGIN_DN

Uppercase names are usually reserved for module-level constants. OTOH,
local variables should be lowercase. Also you can instantiate
dna_config_base directly as DN, using 2-member tuples, i. e:

"""
dna_config_base = DN(('cn', 'posix IDs'), ('cn', 'Distributed Numeric
Assignment Plugin'), ('cn', 'plugins'), ('cn', 'config'))
"""

When passing DN object to the formatting functions/operators, it is
automatically converted to string so no need to hold string and DN
object separately. This is done in other places (see function/methods
in replication.py).

3.)

+        for i in range(len(entries)) :
+
+            mod = []
+ if entries[i].single_value.get(DNA_BIND_METHOD) != method:
+                mod.append((ldap.MOD_REPLACE, DNA_BIND_METHOD,
method))
+
+            if entries[i].single_value.get(DNA_CONN_PROTOCOL) !=
protocol:
+                mod.append((ldap.MOD_REPLACE, DNA_CONN_PROTOCOL,
protocol))


please use idiomatic Python when handling list of entries, e.g.:

"""
for entry in entries:
   mod = []
   if entry.single_value.get(DNA_BIND_METHOD) != method:
   ...
"""

4.) I think that this method should in DSInstance class since it deals with directory server configuration. Service is a parent object of all other service installers/configurators and should contain only methods
common to more children.

5.) Since the method is called from every installer, it could be
beneficial to call it in DSInstance.__common_post_setup() as a part of Directory server installation. Is there any reason why this is not the
case?

6.)

+        while attempt != MAX_WAIT:
+            try:
+                entries = conn.get_entries(sharedcfgdn,
scope=ldap.SCOPE_ONELEVEL, filter='dnaHostname=%s' % self.fqdn)
+                break
+            except errors.NotFound:
+                root_logger.debug("So far enable not find DNA shared
config entry for dnaHostname=%s under %s. Retry in 2sec" % (self.fqdn,
sharedcfgdn))
+                attempt = attempt + 1
+                time.sleep(2)
+                continue
+
+        # safety checking
+        # there is no return, if there are several entries, as a
workaround of #5510
+        if len(entries) != 1:

I am quite afraid what would happen if the server does not return any
entries until 30 s timeout. The code will then continue to the
condition which can potentially test an uninitialized variable and
blow up with 'NameError'. This should be handled more robustly, e. g.
raise an exception when a timeout is reached and no entries were
returned.

7.)

+            if len(mod) > 0:

A Pythonic way to test for non-empty container is

"""
if mods:
   # do stuff
"""

since an empty list/dict/set evaluates to False and non-empty
containers are True.


8.)

+                entry = conn.get_entry(entries[i].dn)
+ if entry.single_value.get(DNA_BIND_METHOD) != method:
+                    root_logger.error("Fail to set SASL/GSSAPI bind
method to %s" % (entries[i].dn))
+                if entry.single_value.get(DNA_CONN_PROTOCOL) !=
protocol:
+                    root_logger.error("Fail to set LDAP protocol to
%s" % (entries[i].dn))

rather than re-fetching the modified entry and testing what happened,
you can just catch an exception raised by unsuccessfull mod and log an
error like this:

"""
try:
   conn.modify_s(entry.dn, mod)
except Exception as e:
root_logger.error("Failed to modify entry {}: {}".format(entry, e))
"""

as a matter of fact, if the modify_s operation would fail for some
reason, an ldap exception would be raised and you checks would not
even be executed.

9.)
The debug message on line 219 should read "Unable to find DNA shared
config entry for dnaHostname=%s so far. Retry in 2 sec.". The errors
at the end of the method should have "Failed" instead of "Fail".

Hi Martin,

Finally tested... here is the updated patch. Thanks for you patience


thanks
thierry

Hi Thierry,

the patch works as expected.

8-)
I have some more nitpicks though:

1.) Please fix the following pep8 errors:

http://paste.fedoraproject.org/329086/56397841/

you can check whether you recent commit conforms to PEP8 by running

"git show -U0 | pep8 --diff"

I did it and fixed most of them but what remains is long line. I fixed
some of them but sometime I think it make the code more complex to read.
Should I split those lines anyway ?

There are some lines that are over 120 characters long and that's really too much. See my quick patch which I attached to make the long lines pep8-conformant. I also fixes some redundant parentheses in the format string and replaces raised exception with logger error.

2.)
+        self.step("update DNA shared config entry",
self.update_dna_shared_config)

I would rather change the message to "Updating DNA shared config
entry" since all other messages use continuous tense.

Right. I will do.


3.)
+            else:
+                raise RuntimeError("Could not get dnaHostname entries
in {} seconds".format(max_wait * 2))

Please use root_logger.error() and then return as is used elsewhere in
the method. We do not want a runaway exception crashing upgrade.
ok I understand. I will fix that

Thanks Martin


+ # a server waits for 30s after its startup to update its shared config
+            max_wait = 30
+            for i in range(0, max_wait + 1):

I also realized that we have 30s timeout in the comment here, bu we actually wait for 60 seconds (30 * time.sleep(2)). We should fix the comment as to not confuse our future selves.


Hello Martin

Thanks for correcting that patch, it looks very good.
I will merge it with my patch and  retesting it.

The server is waiting 30sec after plugin startup to create/update the shared config entries.
In fact I preferred to wait more because startup could be delayed a bit.
The routine will complete immediately if the shared entry already exists, else it will usually wait <~30s (because the server is usually started few seconds before the routine is called). In case of failure to create shared config entry, the installation will wait up to 60sec.

I will change the comment to clarify this

thanks
thierry

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Reply via email to