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

[Stanislav Laznicka]

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160129/ba45dc63/attachment.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-stlaz-0011-1-Listing-and-cleaning-RUV-extended-for-CA-suffix.patch
Type: text/x-patch
Size: 5089 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160129/ba45dc63/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-stlaz-0012-4-Automatically-detect-and-remove-dangling-RUVs.patch
Type: text/x-patch
Size: 8948 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160129/ba45dc63/attachment-0001.bin>