[Freeipa-devel] [PATCH 0024] ipa-replica-manage: added --suffix option for certain commands

Fri Apr 22 15:15:38 UTC 2016

On 04/22/2016 01:13 PM, Martin Basti wrote:
>
>
> On 15.04.2016 14:30, Stanislav Laznicka wrote:
>> On 04/13/2016 01:40 PM, Martin Basti wrote:
>>>
>>>
>>> On 06.04.2016 14:04, Stanislav Laznicka wrote:
>>>> On 03/30/2016 04:52 PM, Martin Basti wrote:
>>>>> On 24.03.2016 19:10, Stanislav Laznicka wrote:
>>>>>> On 03/23/2016 08:13 PM, Martin Basti wrote:
>>>>>>> [...]
>>>>>>> Can you please update design
>>>>>>> http://www.freeipa.org/page/V4/Manage_replication_topology_4_4 
>>>>>>> (mainly
>>>>>>> the --suffix option)? Also there are missing clean-ruv and list-ruv
>>>>>>> commands in design, and fix usage at the bottom.
>>>>>>>
>>>>>>> 1)
>>>>>>> I don't understand this expression
>>>>>>> +            if dirman_passwd is None or (
>>>>>>> +               not dirman_passwd and args[0] in 
>>>>>>> cs_enabled_commands):
>>>>>>>
>>>>>>> You already tested if subcommand belongs to cs_enabled_commands few
>>>>>>> lines above, IMO the 'dirman_password is None' expression is 
>>>>>>> enough.
>>>>>> If I understand it well, when empty password is entered, the 
>>>>>> program continues and uses Kerberos credentials for some 
>>>>>> operations. E.g. for list-ruv, if empty password is entered, the 
>>>>>> command would only display RUVs for domain tree but not for the 
>>>>>> CA tree no matter if CA is set up or not (in the current state of 
>>>>>> the patch, after get_ruv refactoring). This here is one possible 
>>>>>> way around this, although the check for non-empty password might 
>>>>>> probably just as well be in get_ruv_both_suffixes.
>>>>>
>>>>> ok
>>>>>>> 2)
>>>>>>> +# tuple of commands that work with ca tree and need Directory 
>>>>>>> Manager
>>>>>>> password
>>>>>>> +cs_enabled_commands = ("list-ruv", "clean-ruv", "abort-clean-ruv")
>>>>>>>
>>>>>>> this variable is used only toi detect if dirman passwd is needed, I
>>>>>>> suggest to rename it to commands_req_dirman_passwd, or something 
>>>>>>> better.
>>>>>>>
>>>>>>> 3)
>>>>>>> Q: Do we need is_cs_set() function?
>>>>>>> A: Yes!
>>>>>>>
>>>>>>> I wanted to give you ultimate NACK, but then I checked how 
>>>>>>> get_ruv code
>>>>>>> works and I changed my mind.
>>>>>>>
>>>>>>> Please write a comment where is_cs_set function is used, why we 
>>>>>>> need
>>>>>>> extra function instead of catching an exception, possibly you 
>>>>>>> can open a
>>>>>>> refactoring ticket.
>>>>>> After the refactoring changes, is_cs_set should not be needed 
>>>>>> anymore, removed it.
>>>>>>>
>>>>>>> Shame:
>>>>>>> 1)
>>>>>>> +        if not test_connection(realm, host, options.nolookup) or\
>>>>>>> Please use parentheses instead of backslash
>>>>>>>
>>>>>>> 2)
>>>>>>> +           args[0] in cs_enabled_commands:
>>>>>>>
>>>>>>> +               not dirman_passwd and args[0] in 
>>>>>>> cs_enabled_commands):
>>>>>>>
>>>>>>> Indentation is not multiplication of 4
>>>>>> Shame on me indeed, fixed it.
>>>>>>>
>>>>>>> Nitpicks (I don't insist on fixing these):
>>>>>>> 1)
>>>>>>> +    if servers.get('ca', None):
>>>>>>>
>>>>>>> None is default
>>>>>>>
>>>>>>> 2)
>>>>>>> +        for (netloc, rid) in servers['ca']:
>>>>>>> parentheses are not needed
>>>>>>>
>>>>>>> 3)
>>>>>>> + print("\t%s: %s" % (netloc, rid))
>>>>>>> Would be nice to use .format() instead of %
>>>>>>>
>>>>>>> Martin^2
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> I changed my mind, ultimate NACK.
>>>>>>> Please fix get_ruv function, is_cs_set will not help. In case 
>>>>>>> there are no RUVs but CA is installed, sys.exit there prevents 
>>>>>>> us from removing RUVs (or any else operation) on domain suffix, 
>>>>>>> and vice versa.
>>>>>>> I propose to move ticket to 4.4 milestone because I would like 
>>>>>>> to avoid breaking something in 4.3, as this will hit many places 
>>>>>>> in ipa-replica-manage.
>>>>>>>
>>>>>>> Please provide the refactoring of get_ruv as separate patch a 
>>>>>>> put these patches on top of it.
>>>>>>>
>>>>>>> Martin2
>>>>>> Did the refactoring. There also seemed to be duplicit code in 
>>>>>> abort_clean_ruv for some reason, removed it (I hope it does not 
>>>>>> break anything, but it seemed rather useless). Also had to change 
>>>>>> the numbers of the patches so that they would apply.
>>>>>
>>>>> NACK
>>>>>
>>>>> * ipa-replica-manage refactoring *
>>>>>
>>>>> 1)
>>>>> Instead of:
>>>>> -        print("Failed to connect to server %s: %s" % (host, e))
>>>>> -        sys.exit(1)
>>>>> +        root_logger.debug("Failed to connect to server {host}: 
>>>>> {err}"
>>>>> +                          .format(host=host, err=e))
>>>>> +        raise RuntimeError(e)
>>>>>
>>>>> I expected
>>>>>
>>>>> -        print("Failed to connect to server %s: %s" % (host, e))
>>>>> -        sys.exit(1)
>>>>> +        root_logger.debug(traceback.format_exc())
>>>>> +        raise RuntimeError("Failed to connect to server {host}: 
>>>>> {err}"
>>>>> +                                      .format(host=host, err=e)))
>>>> Should be fixed now.
>>>>>
>>>>> 2)
>>>>> -        print("No RUV records found.")
>>>>> -        sys.exit(0)
>>>>>
>>>>> Here is exit state 0, so we should not raise error.
>>>>>
>>>>> I think that we should create new nonfatal exception.
>>>> Made a new nonfatal error exception, then. This step was a bit 
>>>> controversial when it comes to get_ruv_both_suffixes because it 
>>>> needs to catch both this new exception and a RuntimeError exception 
>>>> for both trees. As the main idea here was not to stop if either 
>>>> tree is missing (resulting in RuntimeError in get_ruv) or contains 
>>>> no records (NonFatalError), it is only printed that something bad 
>>>> may have happened (or it's debug-logged in case of nonfatal 
>>>> errors). In the end, only NonFatalError exception is raised if no 
>>>> records were found for whatever reason resulting in the script 
>>>> always returning 0.
>>>>>
>>>>> 3)
>>>>> -                print("unable to decode: %s" % ruv)
>>>>> +                root_logger.debug("unable to decode: %s" % ruv)
>>>>> This may break tests, because the logger logs to stderr, leave it 
>>>>> as print for now
>>>>>
>>>>> 4)
>>>>> -    servers = get_ruv(realm, host, dirman_passwd, nolookup)
>>>>> +    try:
>>>>> +        servers = get_ruv(realm, host, dirman_passwd, nolookup)
>>>>> +    except RuntimeError as e:
>>>>> +        print(e)
>>>>> +        sys.exit(1)
>>>>>
>>>>> again we have to print it to stdout for now.
>>>> 3), 4) done, although it might be better to log to stderr from 
>>>> patch number 27 and on since the default behavior is changed anyway.
>>>>>
>>>>> * abort-clean/list/clean-ruv now work for both suffixes *
>>>>>
>>>>> -            if dirman_passwd is None:
>>>>> +            if dirman_passwd is None or (
>>>>> +               not dirman_passwd and args[0] in 
>>>>> dirman_passwd_req_commands):
>>>>>                  sys.exit("Directory Manager password required")
>>>> Done.
>>>>>
>>>>> Please fix other patch accordingly.
>>>>> Martin^2
>>>>
>>>
>>> 1)
>>> +class NonFatalError(Exception):
>>> +    pass
>>>
>>> IMO we should be more specific and call it NoRUVsFound[Exception]
>>>
>>> 2)
>>> Not sure if this i sstill refactoring, it should be separate patch
>>> -            if dirman_passwd is None:
>>> +            if dirman_passwd is None or (
>>> +               not dirman_passwd and args[0] in 
>>> dirman_passwd_req_commands):
>>>
>>> 3)
>>> +def get_ruv_both_suffixes
>>>
>>> I think in case where both suffixes returns RuntimeError we should 
>>> raise RuntimeError too which results into exit with error code.
>>>
>> Please see the latest patches.
> Well, if CA is not installed on replica, it fails, not sure if this is 
> expected behavior of ipa-replica-manage, or if this is related to your 
> current patches.
>
> # ipa-replica-manage -p hesloheslo clean-dangling-ruv
> Failed to obtain information from 'vm-058-051.example.com': no such entry
>
It's actually a bug in clean_dangling_ruvs that was not created in these 
patches. I opened a separate ticket for it: 
https://fedorahosted.org/freeipa/ticket/5840.