<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 29.01.2016 08:42, Stanislav Laznicka wrote: </div> <blockquote cite="mid:56AB17E8.4060804@redhat.com" type="cite"> <meta content="text/html; charset=windows-1252" http-equiv="Content-Type"> On 01/26/2016 06:56 PM, Martin Basti wrote: <blockquote cite="mid:56A7B365.6090608@redhat.com" type="cite"> <meta content="text/html; charset=windows-1252" http-equiv="Content-Type"> <div class="moz-cite-prefix">On 25.01.2016 16:41, Stanislav Laznicka wrote: </div> <blockquote cite="mid:56A64223.4030809@redhat.com" type="cite"> <meta content="text/html; charset=windows-1252" http-equiv="Content-Type"> Hi, Worked those comments into the code. Also added a bit different info message in clean_ruv with ca=True (ipa-replica-manage:430). Also adding stepst to reproduce: 1. Create a master and some replica (3 replicas is a good solution - 1 with CA, 1 without, 1 to be dangling (with CA)) 2. Change domain level to 0 and ipactl restart 3. Remove the "dangling-to-be" replica from masters.ipa.etc and from both ipaca and domain subtrees in mapping tree.config 4. Try to remove the dangling ruvs with the command Cheers, Standa <div class="moz-cite-prefix">On 01/22/2016 01:22 PM, Martin Basti wrote: </div> <blockquote cite="mid:56A21F17.40406@redhat.com" type="cite"> <meta content="text/html; charset=windows-1252" http-equiv="Content-Type"> Hello, I have a few comments PATCH Automatically detect and remove dangling RUVs 1) + # get the Directory Manager password + if options.dirman_passwd: + dirman_passwd = options.dirman_passwd + else: + dirman_passwd = installutils.read_password('Directory Manager', + confirm=False, validate=False, retry=False) + if dirman_passwd is None: + sys.exit('Directory Manager password is required') + + options.dirman_passwd = dirman_passwd IMO you need only else branch here if not options.dirman_password: dirman_passwd = installutils.read_password('Directory Manager', confirm=False, validate=False, retry=False) if dirman_passwd is None: sys.exit('Directory Manager password is required') options.dirman_passwd = dirman_passwd 2) We should use new formatting in new code (more times in code) + sys.exit( + "Failed to get data from '%s' while trying to list replicas: %s" % + (host, e) + ) sys.exit( "Failed to get data from '{host}' while trying to list replicas: {e}".format( host=host, e=e ) ) 3) + # get all masters + masters_dn = DN(('cn', 'masters'), ('cn', 'ipa'), ('cn', 'etc'), + ipautil.realm_to_suffix(realm)) IMO you should use constants: masters_dn = DN(api.env.container_masters, api.env.basedn) 4) + # Get realm string for config tree + s = realm.split('.') + s = ['dc={dc},'.format(dc=x.lower()) for x in s] + realm_config = DN(('cn', ''.join(s)[0:-1])) Can be api.env.basedn used instead of this block of code? 5) + masters = [x.single_value['cn'] for x in masters] .... + for master in masters: is there any reason why not iterate over the keys in info dict? for master_name, master_data/values/whatever in info.items(): master_data['online'] = True Looks better than: info[master]['online'] = True 6) I asked python gurus, for empty lists and dicts, please use [] and {} instead of list() and dict() It is preferred and faster. 7) + if(info[master]['ca']): + entry = conn.get_entry(csreplica_dn) + csruv = (master, entry.single_value.get('nsDS5ReplicaID')) + if csruv not in csruvs: + csruvs.append(csruv) I dont like too much adding tuples into list and then doing search there, but it is as designed However can you use set() instead of list when the purpose of variable is only testing existence? related to: csruvs ruvs offlines clean_list cleaned 8) conn in finally block may be undefined 9) unused local variables clean_list entry on line 570 10) optional, comment what keys means in info structure </blockquote> </blockquote> Hello, 1) I accept your silence as the following code cannot use nothing from api.env + # Get realm string for config tree + s = realm.split('.') + s = ['dc={dc},'.format(dc=x.lower()) for x in s] + realm_config = DN(('cn', ''.join(s)[0:-1])) but then please use: s = ['dc={dc}'.format(dc=x.lower()) for x in s] realm_config = DN(('cn', ','.join(s))) But I still think that api.env.basedn can be used, because it contains the same format as you need realm_config = DN(('cn', api.env.basedn)) </blockquote> Sorry, forgot to mention that in the last email. However, turns out you are right. I didn't think DN works like this but it does, so this is now in it. <blockquote cite="mid:56A7B365.6090608@redhat.com" type="cite"> 2) nitpick ca_dn = DN(('cn', 'ca'), DN(master.dn)) AFAIK can be just ca_dn = DN(('cn', 'ca'), master.dn) </blockquote> My bad, fixed. <blockquote cite="mid:56A7B365.6090608@redhat.com" type="cite"> 3) uber nitpick This is PEP8 valid, but somehow inconsistent with the rest of code and it hit my eyes print('\t\tid: {id}, hostname: {host}' .format(id=csruv[1], host=csruv[0]) ) we use in code print( something1, something2 ) or print(something1, something2) </blockquote> And that shouldn't be there anymore, too :) <blockquote cite="mid:56A7B365.6090608@redhat.com" type="cite"> Otherwise LGTM </blockquote> </blockquote> ACK Pushed to: ipa-4-3: 44018142747e933f7d680c8d7bacfbd883ffddf6 master: c8eabaff9ef49d3f51b02d613c25c09bf155bb3c </body> </html>