<html> <head> <meta content="text/html; charset=windows-1252" http-equiv="Content-Type"> </head> <body text="#000000" bgcolor="#FFFFFF"> <div class="moz-cite-prefix">On 22.02.2016 20:11, Martin Basti wrote: </div> <blockquote cite="mid:56CB5D57.3070300@redhat.com" type="cite"> <meta content="text/html; charset=windows-1252" http-equiv="Content-Type"> <div class="moz-cite-prefix">On 22.02.2016 19:15, Martin Basti wrote: </div> <blockquote cite="mid:56CB5052.5040803@redhat.com" type="cite"> <meta content="text/html; charset=windows-1252" http-equiv="Content-Type"> <div class="moz-cite-prefix">On 22.02.2016 17:05, Martin Basti wrote: </div> <blockquote cite="mid:56CB31E3.7030002@redhat.com" type="cite"> On 19.02.2016 15:02, Alexander Bokovoy wrote: <blockquote type="cite">On Fri, 19 Feb 2016, Petr Vobornik wrote: <blockquote type="cite">On 02/19/2016 11:12 AM, Alexander Bokovoy wrote: <blockquote type="cite">On Fri, 19 Feb 2016, Martin Basti wrote: <blockquote type="cite">WIP patch attached <a moz-do-not-send="true" class="moz-txt-link-freetext" href="https://fedorahosted.org/freeipa/ticket/5665">https://fedorahosted.org/freeipa/ticket/5665</a> </blockquote> Comments inline. <blockquote type="cite">+ # we need to run sidgen task + sidgen_task_dn = DN("cn=sidgen,cn=ipa-sidgen-task,cn=tasks," + "cn=config") + sidgen_tasks_attr = { + "objectclass": ["top", "extensibleObject"], + "cn": ["sidgen"], + "delay": [0], + "nsslapd-basedn": [self.api.env.basedn], + } </blockquote> May be you are better to name this task more uniquely? Something like 'cn=generate domain sid,cn=...'? <blockquote type="cite">+ + task_entry = ldap.make_entry(sidgen_task_dn, + **sidgen_tasks_attr) + try: + ldap.add_entry(task_entry) + except errors.DuplicateEntry: + self.log.debug("sidgen task already created") + else: + self.log.debug("sidgen task has been created") </blockquote> There could be multiple tasks running in parallel, that's why it could be good to use a different and unique name. <blockquote type="cite">+ # we have to check all trusts domains which have been added after the + # upgrade that caused bug was done. + + base_dn = DN(self.api.env.container_adtrusts, self.api.env.basedn) + trust_domain_entries, truncated = ldap.find_entries( + base_dn=base_dn, + scope=ldap.SCOPE_ONELEVEL, + attrs_list=[attr_name, "cn"], + ) + + if truncated: + self.log.warning("update_sids: Search results were truncated") + + for entry in trust_domain_entries: + if entry.single_value[attr_name] is None: + domain = entry.single_value["cn"] + self.log.error( + "Your trust to %s is broken. Please re-create it by " + "running 'ipa trust-add' again", domain) + + sysupgrade.set_upgrade_state('sidgen', 'update_sids', False) + return False, () </blockquote> This part looks fine. Basically, a similar check needs to be added to trust_find, trust_show, and may be trust_add. </blockquote> Why trust-add? I'm not a big fan of cluttering existing commands(find, show, mod) with logic to fix one upgrade bug. But I understand a need to communicate it somehow. Would it make sense to move such logic to a separate command, e.g. trust-check/trust-verify? The command can do additional check in a future. </blockquote> No. What is the value of trust-add if it says you 'Trust established and verified' while in reality it didn't? This specific issue is very easy to catch. </blockquote> Patch attached, patch with warning will land soon. <fieldset class="mimeAttachmentHeader"></fieldset> </blockquote> Updated patches attached <fieldset class="mimeAttachmentHeader"></fieldset> </blockquote> I fixed except clause in 416, added patch with user warnings, IMO to have separate search is the cleanest solution here, command is not used often, I would like to avoid any hacks in find and show command Patches attached. I will look on ldapsearch timeouts after restarting DS tomorrow. <fieldset class="mimeAttachmentHeader"></fieldset> </blockquote> Updated patches attached + new patch fixing timeout error </body> </html>