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

[Stanislav Laznicka]

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
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160125/eae8ee81/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/20160125/eae8ee81/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-stlaz-0012-3-Automatically-detect-and-remove-dangling-RUVs.patch
Type: text/x-patch
Size: 9181 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160125/eae8ee81/attachment-0001.bin>