[Freeipa-devel] [PATCH 0024] ipa-replica-manage: added --suffix option for certain commands
Martin Basti
mbasti at redhat.com
Thu Apr 28 15:33:04 UTC 2016
On 25.04.2016 10:39, Stanislav Laznicka wrote:
> On 04/22/2016 05:15 PM, Stanislav Laznicka wrote:
>> 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.
>>
> Please see the updated patches. I added verbose flag to
> get_ruv_both_suffixes and made list-ruv print that RUVs were not found
> in either of the trees instead of empty output for that tree.
ACK
ipa-4-3:
* 41458ab80330cc94ea3d7b1da3fe629b882f4356 replica-manage: fail nicely
when DM psswd required
* cf0fbbae8e2a603424d77d52ccaaf428bca1a233 ipa-replica-manage refactoring
* 1ee1ee2d1ec70d8d15884eafa0472edb63ed70d3 abort-clean/list/clean-ruv
now work for both suffixes
* c5f135bf1b16ea3c27d5b3b62c5b751850e0df70 Moved password check from
clean_dangling_ruv
master:
* 37865aa1d7d388042a3b8a66975b466ec27a3d38 replica-manage: fail nicely
when DM psswd required
* d2bb8b7bb1032bf501c984f26f47ef6778c6796a ipa-replica-manage refactoring
* ee05442e5d65766774c18679af78f21a12309730 abort-clean/list/clean-ruv
now work for both suffixes
* c34af691def03313b61a231b85213c8f20e44cfa Moved password check from
clean_dangling_ruv
More information about the Freeipa-devel
mailing list