[Freeipa-devel] [PATCH 0011-0012][RFE] ipa-replica-manage: automatically clean dangling RUVs

[Martin Basti]

On 29.01.2016 08:42, Stanislav Laznicka wrote:
> On 01/26/2016 06:56 PM, Martin Basti wrote:
>>
>>
>> On 25.01.2016 16:41, Stanislav Laznicka wrote:
>>> 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
>>>
>>>
>>> On 01/22/2016 01:22 PM, Martin Basti wrote:
>>>> 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
>>>>
>>>
>> 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))
>>
> 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.
>> 2) nitpick
>> ca_dn = DN(('cn', 'ca'), DN(master.dn))
>>
>> AFAIK can be just
>>
>> ca_dn = DN(('cn', 'ca'), master.dn)
>>
> My bad, fixed.
>> 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)
>>
> And that shouldn't be there anymore, too :)
>> Otherwise LGTM
>

ACK

Pushed to:
ipa-4-3: 44018142747e933f7d680c8d7bacfbd883ffddf6
master: c8eabaff9ef49d3f51b02d613c25c09bf155bb3c

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160202/365e112a/attachment.htm>