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

Fri Apr 22 11:13:13 UTC 2016


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