<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/25/2016 07:17 PM, thierry bordaz wrote: </div> <blockquote cite="mid:56CF452E.30503@redhat.com" type="cite"> <meta content="text/html; charset=windows-1252" http-equiv="Content-Type"> <div class="moz-cite-prefix">On 02/25/2016 12:03 PM, Martin Babinsky wrote: </div> <blockquote cite="mid:56CEDF98.7060809@redhat.com" type="cite">On 02/24/2016 04:30 PM, thierry bordaz wrote: <blockquote type="cite">On 01/21/2016 05:04 PM, Martin Babinsky wrote: <blockquote type="cite">On 01/21/2016 01:37 PM, thierry bordaz wrote: <blockquote type="cite"> </blockquote> Hi Thierry, I have couple of comments to your patch: 1.) there is a number of PEP8 errors in the patch (<a moz-do-not-send="true" class="moz-txt-link-freetext" href="http://paste.fedoraproject.org/313246/33893701">http://paste.fedoraproject.org/313246/33893701</a>), please fix them. See <a moz-do-not-send="true" 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 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". </blockquote> Hi Martin, Finally tested... here is the updated patch. Thanks for you patience thanks thierry </blockquote> Hi Thierry, the patch works as expected. I have some more nitpicks though: 1.) Please fix the following pep8 errors: <a moz-do-not-send="true" class="moz-txt-link-freetext" href="http://paste.fedoraproject.org/329086/56397841/">http://paste.fedoraproject.org/329086/56397841/</a> you can check whether you recent commit conforms to PEP8 by running "git show -U0 | pep8 --diff" 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. 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. </blockquote> Hi Martin, Updated/tested the patch with your help/recommendations. pep8 is clear now :-) thanks thierry </blockquote> Hi Martin, Following your recommendation it is an updated patch to not check/update shared config entry in DSinstance.__post_common_setup(). In fact at this step, DNA plugin is disabled and such the check would be a no-op. thanks thierry </body> </html>